From 785487c52ad175daed04098524d9cfaa1a460dc4 Mon Sep 17 00:00:00 2001 From: ausimian Date: Sun, 23 Aug 2026 13:59:31 +1000 Subject: [PATCH 1/6] feat: install restart transitions under an external supervisor release_handler calls heart:set_cmd/1 while preparing any transition that reboots the emulator, and it writes the installed version to releases/new_start_erl.data while deliberately leaving releases/start_erl.data - what the stock launcher reads - naming the version that is still permanent. So on a Mix release the install failed before rebooting, and past that the reboot would have come back on the version being upgraded away from. install/4 now classifies the transition from the relup before release_handler is asked for anything, and arms releases/castle-restart-pending with the target version when a reboot is coming. Forecastle's env.sh fragment consumes that marker together with OTP's own, requires the two to name one version, and re-execs the stock launcher on it. The two halves are useless apart; forecastle#10 lands with this. Two markers rather than one because new_start_erl.data is written before the reboot and nothing ever removes it, so a preparation that failed after writing it leaves a file naming a version that was never installed - transform_release/3 reconciles the release record and not the file. The marker is cleared on every failing path, and an install that cannot arm one is refused rather than performed: the alternative is a reboot that loses the upgrade with nothing saying so. It has to be a prediction rather than a reaction. A one-stage restart_emulator is replied to with {ok, Vsn, Descr}, exactly as a completed hot upgrade is, and init:reboot() has already been called by the time the reply arrives - so a marker cleared on {ok, ...} would race the shutdown. which_releases/0 is still asked once: the record check and the classification are both about the release the system is running. The two-stage restart_new_emulator is deliberately not armed for. The marker OTP writes for it names the temporary hybrid release, whose version directory holds a start.boot and a sys.config and none of the launcher's own files, so there is nothing there for a launcher to boot. What the install reports changes with it: that the version was installed and the emulator is restarting, and that it stays provisional until it is committed, rather than "Now running" - which is false for as long as the reboot takes, and which automation reads. commit/1 needed nothing, and that is measured rather than assumed: transform_release/3 writes the tmp_current record back as unpacked on disk while set_current/2 makes it current in memory, which is exactly the status do_make_permanent/2 accepts. Refs: https://github.com/ausimian/forecastle/issues/10 Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN --- AGENTS.md | 143 ++++++++++++++++-- RELEASE.md | 44 +++++- lib/castle.ex | 2 +- lib/castle/commands.ex | 264 ++++++++++++++++++++++++++++++---- test/castle/commands_test.exs | 187 ++++++++++++++++++++++-- 5 files changed, 584 insertions(+), 56 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 9af7c21..033fd77 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -552,6 +552,16 @@ Castle's job is configuration and release management on a running node. that commits straight after installing would make a version that cannot boot the permanent one. + A version the launcher booted *provisionally*, after a transition that + restarted the emulator, arrives here as `current` and needs nothing of its + own — which is measured rather than assumed. + `prepare_restart_new_emulator/7` persists it as `tmp_current` before the + reboot; on the boot that follows, `transform_release/3` writes that back as + `unpacked` **on disk** while `set_current/2` hands the handler a record in + which it is `current` **in memory**, because `init:script_id()` names it. So + the same two conditions answer the same question across a reboot, which is + what lets `bin/castle install` poll through one. + The marker is the whole of the evidence, so it inherits whatever the selected boot script does with it. `RELEASE_BOOT_SCRIPT` naming a hand-written script that never reaches `{progress, started}` will never be confirmed — `install` @@ -564,6 +574,72 @@ Castle's job is configuration and release management on a running node. marker ruled this out. It does not: that builds the hybrid script for an emulator upgrade and says nothing about a script an operator supplies.) +- **The restart marker** — `releases/castle-restart-pending`, armed by + `install/4` before `install_release/1` is asked for anything and cleared on + every path where the install failed. Forecastle's `env.sh` fragment consumes + it on the next start and boots the version it names. The two halves are + useless apart and landed together + ([#14](https://github.com/ausimian/castle/issues/14) and + [forecastle#10](https://github.com/ausimian/forecastle/issues/10)). + + **Two files are the evidence, not one, and that is the point of this marker + existing at all.** `prepare_restart_new_emulator/7` writes + `releases/new_start_erl.data` *before* the reboot and nothing ever removes it — + `transform_release/3` reconciles the release *record* and does not touch the + file — so a preparation that failed after writing it leaves a file naming a + version that was never installed. On its own that file is not a boot + instruction. Castle's marker says a reboot was really asked for; the hook + requires both, and requires them to name one version. + + Consuming them is Forecastle's, and both are consumed: the pending marker is + claimed by rename, atomically, and OTP's is removed with it, so the selection + is one-shot and a second start of the same deployment selects nothing. + + **It is armed from the relup, and it has to be, because the reply cannot answer + the question.** `restart_emulator` is replied to with `{ok, Vsn, Descr}` — + exactly what a completed hot upgrade replies — and `init:reboot()` has already + been called by the time the reply arrives. So a marker armed unconditionally + and cleared on `{ok, ...}` would be racing a shutdown, and losing that race + loses the upgrade silently. `restart_planned?/3` therefore reads the same file + `release_handler` will read: `do_get_rh_script/4` looks for the from-version in + the target's own relup and then for the to-version in the from-release's + downgrade section, and the from-version is the release the system is running, + which is what `get_latest_release/1` selects and what `running_release/1` + already computes. **`which_releases/0` is asked once** and the answer used for + both the record check and this, for the reason the record check lives inside + the operation: two calls are two moments. + + This is a prediction and not a second state machine. Nothing here writes a + release record, and it decides exactly one thing — whether to arm. Being wrong + either way is bounded: unarmed, the reboot returns on the permanent version and + `install` reports that the version never became the running one; armed without + a reboot, the hook consumes the marker on the next start and finds nothing to + correlate it with. + + **The two-stage `restart_new_emulator` is deliberately not armed for**, and the + reason is not that it is out of scope. The marker OTP writes for it names the + *temporary hybrid* release, `__new_emulator__`, and + `new_emulator_make_hybrid_boot/6` gives that version directory a `start.boot` + and a `sys.config` and none of the launcher's own furniture — no `env.sh`, no + `elixir`, no `vm.args`. There is nothing there for a launcher to boot, so + arming would point it at a version it cannot start. It is told apart the way + `do_install_release/3` tells them apart: the instruction at the *head* of the + script. Anywhere else it is an error rather than a transition — + `syntax_check_script/1` accepts only `restart_emulator` after the point of no + return. + + **A marker that cannot be written refuses the install**, before + `install_release/1` is asked for anything, because the alternative is a reboot + that comes back on the version being upgraded away from with nothing saying so. + Clearing it, by contrast, is best-effort on purpose: a releases directory the + marker cannot be removed from is one it could not have been written into, and + that already refused. + + **The report changes with it.** `installed/4` says the version was installed, + that the emulator is restarting, and that the version stays provisional until + it is committed — instead of "Now running", which is false for as long as the + reboot takes and which automation reads. + Every one of them is a command entry point, so `Castle` is the command boundary: an operation that fails raises `Castle.Error` there, which is what leaves a non-zero exit status behind for the shell that asked for it. Raising, @@ -780,25 +856,72 @@ It is paired with a release whose check is satisfied, so that "refuses everything" cannot pass for "checks correctly", and with one carrying a shape Elixir does not produce, which has to be refused rather than skipped. +The restart marker's tests are written the same way, and the discriminators are +the same kind: `restart_planned?/3` is exercised by writing a real relup where +`release_handler` reads one and asserting on the *file* — armed for a script +carrying `restart_emulator`, from the from-release's downgrade section for a +downgrade, and not armed for a hot script, for `restart_new_emulator` at the +head, or when there is no relup at all. A relup with no entry for the running +version is what makes the downgrade case a genuine second lookup rather than the +first one succeeding by accident. The arming refusal is arranged by putting a +*directory* at the marker's name, which is deterministic and needs no file modes +— a fixture here may not turn on a mode that root or a filesystem can ignore — +and what it stands for is a releases directory that cannot be written to. And +`Stub.calls(:which_releases) == [[]]` on the successful install is now load +bearing twice over: it says the record check happened in the call that acted, +*and* that the classification did not ask a second time. + What is *not* covered here is a booted release: the upgrade of a running system, and the exit statuses `bin/castle` returns, belong to Forecastle's `:e2e` suite ([#8](https://github.com/ausimian/castle/issues/8)), which exercises this code against a real release and asserts on the success messages each command prints. Those strings — `Unpacked ok`, `Now running (previously ).`, `Committed . …` and the -`releases/0` table — are a contract with that suite. Failure messages are not. +`releases/0` table — are a contract with that suite. Failure messages are not, +and neither is what a restart install reports: that message may not outlive the +reboot it announces, so the `:e2e` suite asserts the exit status and the state +the system ends up in and leaves the wording to the unit test here. + +The restart transition is covered end to end all the same, in both halves: +Forecastle's `restart_upgrade_test.exs` drives a `--restart` relup through +`unpack`/`install`/`commit` against a real supervised release, asserts the OS pid +changes, kills the provisional release before committing to see it roll back, and +installs again from the release that came back. + +It is also what finally covers the interaction between a materialised +`sys.config` and a later cold boot of the same version, which used to be listed +below as unverified. The provisional boot *is* that cold boot: it re-runs the +target's own providers over the materialised file, with `SAMPLE_GREETING` changed +underneath it, and answers with the new value. What lets it is that materialising +leaves no `config_provider_booted` marker behind and preserves the header Mix +wrote, so Elixir's pipeline is still armed in the file the launcher reads. ## Known limitations -- **How the materialised `sys.config` and a later cold boot of the same version - interact is not verified.** Both write the same file. Materialisation resolves - from `sys.config.pristine` and leaves no `config_provider_booted` marker - behind, so a cold boot re-runs the providers over the materialised result — - which is what the issue expects, and what the header Mix wrote is preserved - for. That is now reachable, since - [forecastle#6](https://github.com/ausimian/forecastle/issues/6) landed, but - nothing asserts it: Forecastle's `:e2e` suite installs and commits without - restarting afterwards. It belongs there, because it takes a booted release. +- **Nothing in the release restarts it after an emulator restart, and that is + the design.** `init:reboot()` takes the OS process down, `bin/start` is inert, + and `HEART_COMMAND` is unset, so the external supervisor is the only thing that + brings the system back. A deployment started by hand from a shell therefore + stays down until somebody starts it — and comes back on the installed version + when they do, because the markers are still waiting. See forecastle#10 for why + a second restart authority was refused rather than added. +- **A hard kill between arming the restart marker and completing the install + leaves both markers behind**, and the next start then boots the target. The + damage is bounded rather than absent: the target is unpacked with its + configuration materialised, so what happens is a cold boot of it rather than a + half-applied upgrade, and `releases/start_erl.data` still names the previous + permanent version, so the restart after that returns. Bounded is not closed. + + There is no test that plants the pair by hand, and that is a decision rather + than an omission. Both halves of the bound are already asserted by the restart + `:e2e` suite — the provisional boot of a `restart_emulator` target *is* a cold + boot of it, and killing it before the commit is the case that shows + `start_erl.data` bringing the previous version back. What a planted pair would + add is a state OTP's records contradict: forging the markers for a version the + handler has no `tmp_current` for leaves the node running one release while + `which_releases/0` reports another, and pinning that would be pinning an + incoherence rather than a property. If this is ever closed, the thing to change + is what the marker carries, not what a test plants. - **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)). diff --git a/RELEASE.md b/RELEASE.md index 92ecaeb..4334ba9 100644 --- a/RELEASE.md +++ b/RELEASE.md @@ -112,11 +112,45 @@ script that emits the marker before its applications are started defeats the check instead, since the marker is all there is to go on. - Nothing can build a relup that restarts the emulator until - [forecastle#4](https://github.com/ausimian/forecastle/issues/4), so the - restart transitions this addresses cannot be exercised end to end yet. The - hot-upgrade path is covered by Forecastle's end-to-end suite; the statuses - themselves are covered by unit tests here. + Both the hot-upgrade path and the emulator-restart path are covered end to end + by Forecastle's `:e2e` suite, which polls through a real reboot. +- Upgrades that restart the emulator now work on a release supervised by + systemd, Docker, Kubernetes or anything else that owns starting the service. + `Castle.install/1` recognises such a transition from the relup before it asks + `:release_handler` for anything, and leaves a marker beside the release records + naming the version being installed. The launcher's `env.sh` fragment, which + Forecastle 1.0.0 contributes, consumes that marker on the next start and boots + the version it names. + + Two files have to agree for that to happen, and the reason is worth stating: + `:release_handler` writes `releases/new_start_erl.data` *before* the reboot and + nothing ever removes it, so a preparation that failed part-way leaves a file + naming a version that was never installed. Castle's own marker is what says a + reboot was really asked for; it is written immediately before the install and + removed on every path where the install failed, and the launcher requires both + files and requires them to name one version. An install whose marker cannot be + written - a release root nothing may write to - is refused rather than + performed, because the alternative is a reboot that silently comes back on the + version it was upgrading away from. + + 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 + says that the version was installed, that the emulator is restarting, and that + the version stays provisional until it is committed - which is what + `releases/start_erl.data` still naming the previous version means. `bin/castle + install` goes on asking the system what it is running across the reboot, and + exits 0 once the installed version answers. + + The rollback that provisional state buys is real and needs nothing: + `make_permanent/1` is the only thing that writes `releases/start_erl.data`, so + a provisional release that dies before `Castle.commit/1` is followed by an + ordinary start of the version that was permanent before. + + The two-stage `restart_new_emulator` transition remains unsupported. It reboots + into a temporary hybrid release whose version directory holds a boot script and + a configuration and none of the launcher's own files, so there is nothing for a + launcher to boot; Forecastle refuses to generate one. ### Changed diff --git a/lib/castle.ex b/lib/castle.ex index 0ce3341..7c63cff 100644 --- a/lib/castle.ex +++ b/lib/castle.ex @@ -44,7 +44,7 @@ defmodule Castle do def install(vsn) when is_binary(vsn) do materialise(vsn) - report!(Commands.install(vsn)) + report!(Commands.install(vsn, rel_dir())) end def running(vsn) when is_binary(vsn) do diff --git a/lib/castle/commands.ex b/lib/castle/commands.ex index 1b807a2..76f615f 100644 --- a/lib/castle/commands.ex +++ b/lib/castle/commands.ex @@ -5,6 +5,27 @@ defmodule Castle.Commands do # differ; it has said nothing, which is a different answer. @no_inode "the filesystem reports no inode numbers, so the two cannot be compared" + # The file `install/4` arms before a transition that reboots the emulator, and + # the launcher's `env.sh` fragment consumes on the next start. It sits beside + # the release records, and it is named to be unmistakable: nothing else writes + # it, and a human who finds one knows what it is for. + # + # It exists because `releases/new_start_erl.data` on its own is not evidence. + # `prepare_restart_new_emulator/7` writes that file *before* the reboot and + # nothing ever removes it, so a preparation that failed after writing it - and + # `transform_release/3` reconciles the release record without touching the + # marker - leaves a file naming a version that was never installed. The two + # together are the evidence: OTP's marker says which version, and this one says + # that Castle asked for the reboot that would boot it. The hook requires both, + # and requires them to name the same version. + @restart_marker "castle-restart-pending" + + # The two instructions `release_handler` treats as "reboot the emulator", and + # what `do_install_release/3` does with each. Only the one-stage instruction is + # a transition the launcher can select the target of; see `restart_planned?/3`. + @one_stage :restart_emulator + @two_stage :restart_new_emulator + # The implementation of each of Castle's commands, held apart from the # command boundary in `Castle` so that it can be exercised without a booted # release: @@ -333,7 +354,16 @@ defmodule Castle.Commands do # it is the unpack or the install refusing, and what they need told is that it # did not happen. defp ensure_upgradable(refusal, handler) do - case running_release(handler) do + refuse_synthesised(refusal, running_release(handler)) + end + + # The same rule over a running release that has already been asked for. + # `install/4` needs it twice - the record check, and which relup entry the + # transition will be evaluated from - and `which_releases/0` must be asked once: + # two calls are two moments, which is the whole point of the check being inside + # the operation. + defp refuse_synthesised(refusal, running) do + case running do {_vsn, [_ | _]} -> :ok @@ -477,34 +507,198 @@ defmodule Castle.Commands do upgraded at all, so there is no upgrade under way for it to strand, and what those operations would otherwise act on is the Erlang installation. Only the read-only `upgradable/1` and `releases/1` are without it. + + `rel_dir` is where the restart marker is armed. It is an argument for the + reason `make_releases/3`'s is: nothing chooses it, `Castle.install/1` derives + it from `code:root_dir()`, and a test needs somewhere to look. """ - @spec install(String.t(), module(), module()) :: result() - def install(vsn, handler \\ :release_handler, deployment \\ Castle.Deployment) do - with :ok <- ensure_own_erts("Cannot install #{vsn}", deployment), - :ok <- ensure_upgradable("Cannot install #{vsn}", handler) do - case handler.install_release(to_charlist(vsn)) do - {:ok, other_vsn, _descr} -> - {:ok, ["Now running #{vsn} (previously #{other_vsn})."]} - - # The emulator, or one of kernel, stdlib and sasl, is being replaced, so - # the node reboots and the upgrade instructions run after it comes back. - # Nothing has failed. - {:continue_after_restart, other_vsn, _descr} -> - {:ok, - [ - "Restarting to install #{vsn} (previously #{other_vsn}).", - "The upgrade continues once the emulator has restarted." - ]} - - {:error, reason} -> - {:error, "Install of #{vsn} failed. #{inspect(reason)}"} - - other -> - {:error, "Install of #{vsn} returned an unexpected result. #{inspect(other)}"} - end + @spec install(String.t(), Path.t(), module(), module()) :: result() + def install(vsn, rel_dir, handler \\ :release_handler, deployment \\ Castle.Deployment) do + with :ok <- ensure_own_erts("Cannot install #{vsn}", deployment) do + install_upgradable(vsn, rel_dir, handler) + end + end + + # Everything after the ERTS guard, with the running release asked for once. + # + # The record check and the restart prediction are both about the release the + # system is running - `get_latest_release/1` is `current` if there is one and + # `permanent` otherwise, which is what `running_release/1` computes - and asking + # `which_releases/0` twice would be asking about two moments. + defp install_upgradable(vsn, rel_dir, handler) do + refusal = "Cannot install #{vsn}" + running = running_release(handler) + restart? = restart_planned?(vsn, rel_dir, running) + + with :ok <- refuse_synthesised(refusal, running), + :ok <- arm_restart(restart?, vsn, rel_dir, refusal) do + installed(vsn, restart?, rel_dir, handler) + end + end + + # `install_release/1` replies the same `{ok, Vsn, Descr}` for a hot upgrade and + # for one that is about to reboot, so what it says cannot tell them apart - + # which is why the transition is classified from the relup beforehand and the + # answer carried in here. Reporting a reboot as "now running" is a claim that is + # false for as long as the reboot takes, and automation reads it. + defp installed(vsn, restart?, rel_dir, handler) do + case handler.install_release(to_charlist(vsn)) do + {:ok, other_vsn, _descr} when restart? -> + {:ok, + [ + "Installed #{vsn} (previously #{other_vsn}). The emulator is restarting.", + "#{vsn} is provisional until it is committed: #{other_vsn} is still the " <> + "version an ordinary restart boots." + ]} + + {:ok, other_vsn, _descr} -> + {:ok, ["Now running #{vsn} (previously #{other_vsn})."]} + + # The emulator, or one of kernel, stdlib and sasl, is being replaced, so + # the node reboots and the upgrade instructions run after it comes back. + # Nothing has failed here - but nothing selects the hybrid temporary + # release the reboot needs either, which is why no marker was armed for it. + # See `restart_planned?/3`. + {:continue_after_restart, other_vsn, _descr} -> + {:ok, + [ + "Restarting to install #{vsn} (previously #{other_vsn}).", + "The upgrade continues once the emulator has restarted." + ]} + + {:error, reason} -> + disarm(restart?, rel_dir) + {:error, "Install of #{vsn} failed. #{inspect(reason)}"} + + other -> + disarm(restart?, rel_dir) + {:error, "Install of #{vsn} returned an unexpected result. #{inspect(other)}"} + end + end + + ## The restart marker + + # Whether the transition about to be installed reboots the emulator into a + # version the launcher can be told to boot. + # + # This is a prediction, and it is made from the same file `release_handler` + # will read: `do_get_rh_script/4` looks for the from-version in the target's + # own relup and then for the to-version in the from-release's, which is what + # `transition_script/3` does. It is not a second implementation of the state + # machine - nothing here writes a release record - it decides one thing, which + # is whether to arm the marker, and OTP remains authoritative for everything + # else. + # + # It has to be a prediction because the reply cannot answer it. A one-stage + # restart is replied to with `{ok, Vsn, Descr}`, exactly as a completed hot + # upgrade is, and `init:reboot()` has already been called by the time the reply + # arrives - so a marker armed unconditionally and cleared on `{ok, ...}` would + # be racing the shutdown, and losing that race silently loses the upgrade. + # + # The two instructions are told apart the way `do_install_release/3` tells them + # apart. `restart_new_emulator` at the head of the script is the two-stage + # transition, and it is deliberately *not* armed for: the marker OTP writes then + # names the temporary hybrid release, `__new_emulator__`, whose version + # directory holds a `start.boot` and a `sys.config` and none of the launcher's + # own furniture - no `env.sh`, no `elixir`, no `vm.args` - so there is nothing + # for the launcher to boot. That transition is unsupported rather than + # half-supported; Forecastle refuses to generate one. + # + # Anywhere else in the script, `restart_new_emulator` is an error rather than a + # transition: `syntax_check_script/1` accepts only `restart_emulator` after the + # point of no return, and `eval/2` throwing the other atom lands in + # `eval_script/5`'s error branch. So it never reaches a reboot, and the install + # fails with the marker unarmed. + defp restart_planned?(_to_vsn, _rel_dir, nil), do: false + + defp restart_planned?(to_vsn, rel_dir, {from_vsn, _apps}) do + case transition_script(rel_dir, to_vsn, from_vsn) do + [@two_stage | _] -> false + script when is_list(script) -> @one_stage in script + nil -> false + end + end + + # The relup entry `release_handler` will evaluate, or `nil` if there is not one + # it can find either. A missing or unreadable relup is not this function's to + # report: `do_get_rh_script/4` throws `no_matching_relup` for it, the install + # fails, and the failure names the release rather than a marker nobody asked + # about. + defp transition_script(rel_dir, to_vsn, from_vsn) do + upgrade_script(rel_dir, to_vsn, from_vsn) || downgrade_script(rel_dir, to_vsn, from_vsn) + end + + defp upgrade_script(rel_dir, to_vsn, from_vsn) do + case relup(rel_dir, to_vsn) do + {^to_vsn, ups, _downs} -> script_for(ups, from_vsn) + _other -> nil + end + end + + defp downgrade_script(rel_dir, to_vsn, from_vsn) do + case relup(rel_dir, from_vsn) do + {^from_vsn, _ups, downs} -> script_for(downs, to_vsn) + _other -> nil + end + end + + # Versions come back from a relup as charlists, so they are compared as + # strings - the same normalisation `running_release/1` and every message here + # already apply. + defp relup(rel_dir, vsn) do + case :file.consult(to_charlist(Path.join([rel_dir, vsn, "relup"]))) do + {:ok, [{relup_vsn, ups, downs}]} when is_list(ups) and is_list(downs) -> + {to_string(relup_vsn), ups, downs} + + _unreadable -> + nil + end + end + + defp script_for(entries, vsn) do + Enum.find_value(entries, fn + {from, _descr, script} when is_list(script) -> if to_string(from) == vsn, do: script + _malformed -> nil + end) + end + + # Armed before `install_release/1` is asked for anything, so that the marker is + # in place before OTP writes its own and reboots. A failure to write it refuses + # the install rather than going ahead: the reboot would come back on whichever + # version `releases/start_erl.data` names, which is the one being upgraded + # away from, and the upgrade would be lost with nothing saying so. + defp arm_restart(false, _vsn, _rel_dir, _refusal), do: :ok + + defp arm_restart(true, vsn, rel_dir, refusal) do + marker = Path.join(rel_dir, @restart_marker) + + case File.write(marker, vsn <> "\n") do + :ok -> + :ok + + {:error, reason} -> + {:error, + "#{refusal}: the upgrade to #{vsn} restarts the emulator, and #{marker} - " <> + "which is what tells the launcher to boot #{vsn} when the system comes back - " <> + "could not be written (#{:file.format_error(reason)}). Without it the restart " <> + "would come back on the version releases/start_erl.data names, losing the " <> + "upgrade."} end end + # Cleared on every path out of a failed install, because a marker left armed + # beside a `new_start_erl.data` the same failure may already have written is + # exactly the pair the hook acts on. + # + # The result is deliberately not looked at, and that is not laziness: a + # releases directory this cannot remove the marker from is one `arm_restart/4` + # could not have written it into, and that refuses the install before + # `install_release/1` is asked for anything. So reaching here at all means the + # marker was writable moments ago, and there is no state left worth branching + # on - only a state nothing could produce and no test could arrange. + defp disarm(false, _rel_dir), do: :ok + defp disarm(true, rel_dir), do: File.rm(Path.join(rel_dir, @restart_marker)) + @doc """ Confirms that `vsn` is the release the system is running. @@ -522,6 +716,13 @@ defmodule Castle.Commands do target as, and `:tmp_current`, which is written before the reboot a restart transition has yet to make. + A version the launcher booted provisionally, after a restart transition, + arrives here as `:current` too and needs nothing of its own: `transform_release/3` + writes the `tmp_current` record back as `unpacked` on disk, and `set_current/2` + makes it `current` in the record the handler holds, because `init:script_id()` + names it. So the same two conditions answer the same question across a reboot, + which is what lets `bin/castle install` poll through one. + Being the running release is necessary but not sufficient, because a node that restarted into it can be seen part-way up. `release_handler` records the new version as `:current` while `sasl` starts, and distribution is already @@ -600,6 +801,19 @@ defmodule Castle.Commands do promoting a version of the Erlang installation. Those two are the *records*, so they are the relocatable half - see `Castle.Deployment.root_dir/0` - but the version it would be promoting is the installation's either way. + + **A version reached by a restart transition needs nothing extra here, and that + is measured rather than assumed.** `prepare_restart_new_emulator/7` persists + the target as `tmp_current` before the reboot, and on the boot that follows + `transform_release/3` writes it back as `unpacked` *on disk* while + `set_current/2` hands the handler a record in which it is `current` in memory - + because `init:script_id()` names it. `do_make_permanent/2` reads the in-memory + record and accepts any status but `unpacked`, `old` and `permanent`, so + `current` is exactly what it wants; `set_permanent_files/5` then writes + `releases/start_erl.data`, and `write_releases/3` corrects the on-disk record. + 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. """ @spec commit(String.t(), module(), module()) :: result() def commit(vsn, handler \\ :release_handler, deployment \\ Castle.Deployment) do diff --git a/test/castle/commands_test.exs b/test/castle/commands_test.exs index bd5daba..3ebf2d4 100644 --- a/test/castle/commands_test.exs +++ b/test/castle/commands_test.exs @@ -159,60 +159,202 @@ defmodule Castle.CommandsTest do end end - describe "install/3" do - test "reports the version change" do + describe "install/4" do + @tag :tmp_dir + test "reports the version change", %{tmp_dir: dir} do handler = real_record(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) - assert Commands.install("1.2.3", handler) == + assert Commands.install("1.2.3", dir, handler) == {:ok, ["Now running 1.2.3 (previously 1.2.2)."]} assert Stub.calls(:install_release) == [[~c"1.2.3"]] end - test "reports a restart of the emulator as the success it is" do + @tag :tmp_dir + test "reports a restart of the emulator as the success it is", %{tmp_dir: dir} do handler = real_record(:install_release, {:continue_after_restart, ~c"1.2.2", ~c"upgrade"}) - assert {:ok, lines} = Commands.install("1.2.3", handler) + assert {:ok, lines} = Commands.install("1.2.3", dir, handler) assert Enum.join(lines, " ") =~ "Restarting to install 1.2.3 (previously 1.2.2)." end - test "reports a failure to install" do + @tag :tmp_dir + test "reports a failure to install", %{tmp_dir: dir} do handler = real_record(:install_release, {:error, {:no_such_release, ~c"1.2.3"}}) - assert {:error, message} = Commands.install("1.2.3", handler) + assert {:error, message} = Commands.install("1.2.3", dir, handler) assert message =~ "Install of 1.2.3 failed." assert message =~ "no_such_release" end - test "reports a result it does not recognise" do + @tag :tmp_dir + test "reports a result it does not recognise", %{tmp_dir: dir} do handler = real_record(:install_release, {:whatever, ~c"1.2.2"}) - assert {:error, message} = Commands.install("1.2.3", handler) + assert {:error, message} = Commands.install("1.2.3", dir, handler) assert message =~ "Install of 1.2.3 returned an unexpected result." end - test "refuses a system running on a record OTP synthesised, without installing" do + @tag :tmp_dir + test "refuses a system running on a record OTP synthesised, without installing", + %{tmp_dir: dir} do # The mutation is install_release/1, and the handler here is ready to # perform it and report success - which is exactly what such an install # would do while leaving applications on their old code. So the refusal has # to come first, and nothing may reach the handler. handler = synthesised_record(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) - assert {:error, message} = Commands.install("1.2.3", handler) + assert {:error, message} = Commands.install("1.2.3", dir, handler) assert message =~ "Cannot install 1.2.3: 1.2.2 is running from a release record" assert message =~ "running its old code" assert message =~ "the system has to be restarted" assert Stub.calls(:install_release) == [] end - test "asks the running node, in the call that does the installing" do + @tag :tmp_dir + test "asks the running node once, in the call that does the installing", %{tmp_dir: dir} do + # Once, and not twice: the record check and the classification that decides + # whether to arm the restart marker are both about the release the system is + # running, and two calls to which_releases/0 are two moments. That is the + # same reason the check lives inside the operation at all. handler = real_record(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) - assert {:ok, _} = Commands.install("1.2.3", handler) + assert {:ok, _} = Commands.install("1.2.3", dir, handler) assert Stub.calls(:which_releases) == [[]] end end + # The marker the launcher's env.sh fragment consumes on the next start, and + # which - together with the `new_start_erl.data` release_handler writes - is + # what makes a reboot boot the version that was installed rather than the one + # `releases/start_erl.data` still names. + # + # It is armed from the relup, before install_release/1 is asked for anything, + # because the reply cannot answer the question: a one-stage restart is replied + # to with the same `{ok, Vsn, Descr}` a completed hot upgrade is, and the reboot + # has already been asked for by then. + describe "the restart marker" do + @tag :tmp_dir + test "is armed for a one-stage restart transition", %{tmp_dir: dir} do + relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) + handler = real_record(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) + + assert {:ok, lines} = Commands.install("1.2.3", dir, handler) + assert File.read!(marker(dir)) == "1.2.3\n" + + # And the report says what happened rather than what a hot upgrade's reply + # would have suggested: the same {ok, ...} means the emulator is going down. + report = Enum.join(lines, " ") + assert report =~ "Installed 1.2.3 (previously 1.2.2). The emulator is restarting." + assert report =~ "1.2.3 is provisional until it is committed" + assert report =~ "1.2.2 is still the version an ordinary restart boots" + refute report =~ "Now running" + end + + @tag :tmp_dir + test "is armed from the from-release's relup for a downgrade", %{tmp_dir: dir} do + # do_get_rh_script/4 looks for the from-version in the target's relup and, + # failing that, for the to-version in the from-release's downgrade section. + # A relup for 1.2.3 that has no entry for 1.2.2 and a relup for 1.2.2 whose + # downgrade section names 1.2.1 is the second case. + relup!(dir, "1.2.3", [], []) + relup!(dir, "1.2.2", [], [{~c"1.2.1", [], [:restart_emulator]}]) + handler = real_record(:install_release, {:ok, ~c"1.2.2", ~c"downgrade"}) + + assert {:ok, _} = Commands.install("1.2.1", dir, handler) + assert File.read!(marker(dir)) == "1.2.1\n" + end + + @tag :tmp_dir + test "is not armed for a hot upgrade", %{tmp_dir: dir} do + relup!(dir, "1.2.3", [{~c"1.2.2", [], [{:apply, {:m, :f, []}}]}], []) + handler = real_record(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) + + assert {:ok, ["Now running 1.2.3 (previously 1.2.2)."]} = + Commands.install("1.2.3", dir, handler) + + refute File.exists?(marker(dir)) + end + + @tag :tmp_dir + test "is not armed for the two-stage restart", %{tmp_dir: dir} do + # restart_new_emulator at the head of the script is the transition that + # boots a hybrid temporary release, and the marker release_handler writes + # for it names `__new_emulator__` - a version directory with a + # start.boot and a sys.config in it and none of the launcher's own files. + # There is nothing there for the launcher to boot, so arming would point it + # at a version it cannot start. + relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_new_emulator, {:apply, {:m, :f, []}}]}], []) + handler = real_record(:install_release, {:continue_after_restart, ~c"1.2.2", ~c"upgrade"}) + + assert {:ok, _} = Commands.install("1.2.3", dir, handler) + refute File.exists?(marker(dir)) + end + + @tag :tmp_dir + test "is not armed when there is no relup to classify from", %{tmp_dir: dir} do + # do_get_rh_script/4 throws no_matching_relup for this, so the install fails + # and says so. Nothing here has to report it, and nothing may arm on a guess. + handler = + real_record(:install_release, {:error, {:no_matching_relup, ~c"1.2.3", ~c"1.2.2"}}) + + assert {:error, _} = Commands.install("1.2.3", dir, handler) + refute File.exists?(marker(dir)) + end + + @tag :tmp_dir + test "is cleared when the install fails", %{tmp_dir: dir} do + # The case that makes the marker necessary in the first place, seen from the + # other side: prepare_restart_new_emulator/7 writes new_start_erl.data + # before it can fail, and nothing removes it. If the marker survived a + # failure the two would agree, and the next restart of the system would boot + # a version that was never installed. + relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) + handler = real_record(:install_release, {:error, {:bad_relup_file, ~c"relup"}}) + + assert {:error, message} = Commands.install("1.2.3", dir, handler) + assert message =~ "Install of 1.2.3 failed." + refute File.exists?(marker(dir)) + end + + @tag :tmp_dir + test "is cleared when the install answers something unrecognised", %{tmp_dir: dir} do + relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) + handler = real_record(:install_release, {:whatever, ~c"1.2.2"}) + + assert {:error, _} = Commands.install("1.2.3", dir, handler) + refute File.exists?(marker(dir)) + end + + @tag :tmp_dir + test "refuses the install when it cannot be armed", %{tmp_dir: dir} do + # A directory at the marker's name is what makes this deterministic; what it + # stands for is a releases directory that cannot be written to. Going ahead + # would reboot the system and come back on the version start_erl.data names, + # losing the upgrade with nothing saying so - so the install is refused + # before install_release/1 is asked for anything, which is the line + # everything else here is on the right side of too. + relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) + File.mkdir!(marker(dir)) + handler = real_record(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) + + assert {:error, message} = Commands.install("1.2.3", dir, handler) + assert message =~ "Cannot install 1.2.3: the upgrade to 1.2.3 restarts the emulator" + assert message =~ Path.join(dir, "castle-restart-pending") + assert message =~ "losing the upgrade" + assert Stub.calls(:install_release) == [] + end + + @tag :tmp_dir + test "is not armed for a deployment the ERTS guard refuses", %{tmp_dir: dir} do + relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) + handler = real_record(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) + + assert {:error, _} = Commands.install("1.2.3", dir, handler, erts_less()) + refute File.exists?(marker(dir)) + end + end + describe "running/3" do test "confirms the version an install has made current" do handler = @@ -412,10 +554,11 @@ defmodule Castle.CommandsTest do assert Stub.calls(:which_releases) == [] end - test "refuses to install, without installing" do + @tag :tmp_dir + test "refuses to install, without installing", %{tmp_dir: dir} do handler = real_record(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) - assert {:error, message} = Commands.install("1.2.3", handler, erts_less()) + assert {:error, message} = Commands.install("1.2.3", dir, handler, erts_less()) assert message =~ "Cannot install 1.2.3: the deployment and the emulator's root are different directories" @@ -560,4 +703,18 @@ defmodule Castle.CommandsTest do # the peer's business, and the peer is a stub here. Everything it would look # for is covered against a real one in `Castle.PeerTest`. defp unpacked(dir), do: File.write!(Path.join(dir, "sys.config"), "[].\n") + + defp marker(rel_dir), do: Path.join(rel_dir, "castle-restart-pending") + + # A relup where `release_handler` reads one: `releases//relup`, holding a + # single `{Vsn, Ups, Downs}` term. Written as a term rather than as text so that + # what the classification consults is what `:file.consult/1` gives it, which is + # the same thing `do_get_rh_script/4` will be given a moment later. + defp relup!(rel_dir, vsn, ups, downs) do + dir = Path.join(rel_dir, vsn) + File.mkdir_p!(dir) + + plan = {to_charlist(vsn), ups, downs} + File.write!(Path.join(dir, "relup"), :io_lib.format(~c"~tp.~n", [plan])) + end end From b8f74e873af15f3fcc0765dafe72efe5c91c09e7 Mon Sep 17 00:00:00 2001 From: ausimian Date: Sun, 23 Aug 2026 14:57:31 +1000 Subject: [PATCH 2/6] fix: make the restart marker evidence about one install attempt Two files that agree on a version do not establish that one install produced them. prepare_restart_new_emulator/7 writes new_start_erl.data before the reboot and nothing removes it, so a failed attempt to X left OTP's half of the pair behind; a retry to X armed a fresh marker beside it, and a manual or hard restart before the retry reached install_release/1 then presented a matching pair for an install that never happened - the node booted X while which_releases/0 reported it unpacked. Back-to-back or concurrent installs broke it the other way, by overwriting and disarming each other's marker. arm_restart/4 is three steps now, and the order is the protocol. A marker already at the path refuses the install, so there is one pending restart install at a time and the look comes before anything destructive. Any new_start_erl.data is then cleared - safe, because write_new_start_erl/3 goes through file:write_file/2, which creates the file when it is absent - so past that point the file existing means this attempt's own preparation wrote it. The marker is staged in an owner-only working directory and hard-linked into place, the way Castle.Peer publishes sys.config.pristine: a link publishes a file that is already complete, and refuses rather than replaces. An exclusive create in place has neither property - it makes creation atomic and leaves the file empty until the write, and a death there leaves an empty marker that blocks every later attempt. The marker carries the attempt on a second line - operating system pid, wall clock in nanoseconds, serial - and disarm/2 removes it only while it still says so. The name is shared and the marker is short-lived, because any start or daemon of the deployment consumes it whether or not it goes on to boot, so removing it by name would take a later attempt's reboot away. The hook reads only the first line, which is the version, so nothing on the shell side parses anything it did not before. Castle.ReleaseHandlerStub takes a function as a reply now, because the states this rests on exist only while install_release/1 is in flight: a preparation that writes new_start_erl.data and then fails, so a same-version retry can be shown to clear it; the filesystem as a hard restart before the reboot would find it, which is the marker alone; and a marker replaced between the arming and the disarming. The claim that both markers are consumed atomically was false, and is corrected here and in the release note. What is atomic is the claim of Castle's marker; OTP's file is read and removed separately, and no POSIX operation moves two files together. The order is what makes that safe - the marker goes first, so an interruption loses the selection rather than misapplying it. Refs: https://github.com/ausimian/forecastle/issues/10 Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN --- AGENTS.md | 149 +++++++++++--- RELEASE.md | 16 ++ lib/castle/commands.ex | 287 ++++++++++++++++++++++++--- test/castle/commands_test.exs | 184 ++++++++++++++++- test/support/release_handler_stub.ex | 8 + 5 files changed, 568 insertions(+), 76 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 033fd77..fbea156 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -591,9 +591,69 @@ Castle's job is configuration and release management on a running node. instruction. Castle's marker says a reboot was really asked for; the hook requires both, and requires them to name one version. - Consuming them is Forecastle's, and both are consumed: the pending marker is - claimed by rename, atomically, and OTP's is removed with it, so the selection - is one-shot and a second start of the same deployment selects nothing. + **Agreeing on a version is not enough, and the first version of this shipped + believing it was.** Two files that name the same version do not establish that + one install produced them. The sequence that breaks it has no exotic step in + it: an attempt to X fails after OTP's file is written, the operator retries X, + the retry arms a fresh marker beside the stale file, and a manual or hard + restart *before the retry reaches `install_release/1`* then presents a matching + pair. The launcher boots X, which nothing installed, and OTP's records call it + `unpacked` — the node runs one release while `which_releases/0` reports another. + Back-to-back or concurrent installs broke it the other way, by overwriting and + disarming each other's marker. + + **So the pair is owned by an install *attempt*, and three things make it so.** + `arm_restart/4` is the whole of it and the order is the protocol. + + 1. **One pending restart install at a time.** A marker already at the path + refuses the install rather than being adopted or replaced. `publish/2` + decides it, by refusing rather than replacing; the `lstat` first is what + keeps step 2 from destroying a pending attempt's evidence, since anything in + flight holds the marker from before its own step 2 until its `disarm/2`. + 2. **OTP's file is cleared before the marker is armed.** That is what closes + the window above: after it, `new_start_erl.data` existing means *this* + attempt's preparation wrote it. Removing it is safe — + `write_new_start_erl/3` goes through `file:write_file/2`, which creates the + file when it is absent. The order matters and must not be reversed: an + attempt that refused *after* clearing would take a concurrent install's + reboot away silently. + 3. **The marker names the attempt that armed it**, on a second line, and + `disarm/2` removes it only if it still does. The marker's name is shared and + the marker is short-lived — *any* `start` or `daemon` of the deployment + consumes it, whether or not that start goes on to boot — so removing it by + name would take a later attempt's marker away. The attempt is the operating + system pid, the wall clock in nanoseconds and a serial: unique within a node + and across its restarts, which is as far as ownership has to reach, because + the marker never outlives the next start. It is not a secret and does not + need to be — anything able to forge it can write in the releases directory, + where it could write the marker itself. What it defends against is + confusion, not forgery. **The hook never reads it**: the version is the + first line, which is what `head -n 1` gives it, so the file carries this + without the shell parsing anything it did not before. + + **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 + link was chosen there. A link publishes a file that is already complete, so no + start can read a marker that is empty or half written; and it refuses rather + than replaces, so the loser of a race is told instead of silently taking the + marker over. An exclusive create in place has neither property: it makes + *creation* atomic and leaves the file empty between the open and the write, and + a death in that window leaves an empty marker that blocks every later attempt. + `Castle.Commands` therefore calls `Castle.Peer.work_dir/1`, + `write_private/2` and `publish/2` **directly**, not through the injected module + `materialise/3` uses: those start no VM, there is nothing about them a stub + could stand for, and the guarantee is the point. + + Consuming them is Forecastle's, and what is atomic there is the *claim*: the + pending marker is taken by rename, so exactly one start can act on the pair, + and OTP's file is then read and removed separately. The two removals are not + one operation and cannot be made one — no POSIX call renames two files + together. What that costs is bounded by the order: the marker goes first, so an + interruption anywhere after it leaves no marker and the next start boots the + permanent version. The selection is lost, never duplicated, and never applied + to a version nothing installed. Do not write that both are consumed + atomically: this note said it, Forecastle's `AGENTS.md` said it, and + Forecastle's release note said it, and it was false in all three. **It is armed from the relup, and it has to be, because the reply cannot answer the question.** `restart_emulator` is replied to with `{ok, Vsn, Descr}` — @@ -628,12 +688,13 @@ Castle's job is configuration and release management on a running node. `syntax_check_script/1` accepts only `restart_emulator` after the point of no return. - **A marker that cannot be written refuses the install**, before - `install_release/1` is asked for anything, because the alternative is a reboot - that comes back on the version being upgraded away from with nothing saying so. - Clearing it, by contrast, is best-effort on purpose: a releases directory the - marker cannot be removed from is one it could not have been written into, and - that already refused. + **A marker that cannot be armed refuses the install**, before + `install_release/1` is asked for anything, and so does a stale + `new_start_erl.data` that cannot be cleared — because the alternative in both + cases is a reboot that comes back on the wrong version with nothing saying so. + Clearing the marker, by contrast, is best-effort on purpose: a releases + directory the marker cannot be removed from is one it could not have been + linked into, and that already refused. **The report changes with it.** `installed/4` says the version was installed, that the emulator is restarting, and that the version stays provisional until @@ -863,14 +924,32 @@ carrying `restart_emulator`, from the from-release's downgrade section for a downgrade, and not armed for a hot script, for `restart_new_emulator` at the head, or when there is no relup at all. A relup with no entry for the running version is what makes the downgrade case a genuine second lookup rather than the -first one succeeding by accident. The arming refusal is arranged by putting a -*directory* at the marker's name, which is deterministic and needs no file modes -— a fixture here may not turn on a mode that root or a filesystem can ignore — -and what it stands for is a releases directory that cannot be written to. And -`Stub.calls(:which_releases) == [[]]` on the successful install is now load -bearing twice over: it says the record check happened in the call that acted, +first one succeeding by accident. Both arming refusals are arranged by putting a +*directory* where a file has to be — at the marker's name for the one, at +`new_start_erl.data` for the other — which is deterministic and needs no file +modes, since a fixture here may not turn on a mode that root or a filesystem can +ignore. And `Stub.calls(:which_releases) == [[]]` on the successful install is now +load bearing twice over: it says the record check happened in the call that acted, *and* that the classification did not ask a second time. +**The attempt-ownership tests are about states that only exist while +`install_release/1` is in flight**, so `Castle.ReleaseHandlerStub` accepts a +*function* as a reply and calls it with the arguments. That is the only seam +between the arming and the disarming, and it is what makes three otherwise +unobservable things assertable: a preparation that writes `new_start_erl.data` +and *then* fails, so that a same-version retry can be shown to clear it rather +than pair with it; the filesystem as a hard restart before the reboot would find +it, which is the marker alone and no pair; and a marker replaced between the +arming and the disarming, so that `disarm/2` can be shown to leave a marker it +did not write. None of those has an end state that distinguishes it — a +successful install leaves the marker armed either way, and a failed one leaves it +gone either way — which is the same reason `Castle.Peer`'s primitives are public. +The concurrency case needs no seam: a marker already at the path is a pending +attempt, and what is asserted is that the install is refused, that +`install_release/1` was never called, and that *neither* the marker nor OTP's file +was touched — the second half being the ordering that keeps a refusal from +destroying a concurrent install's evidence. + What is *not* covered here is a booted release: the upgrade of a running system, and the exit statuses `bin/castle` returns, belong to Forecastle's `:e2e` suite ([#8](https://github.com/ausimian/castle/issues/8)), which @@ -905,23 +984,29 @@ wrote, so Elixir's pipeline is still armed in the file the launcher reads. stays down until somebody starts it — and comes back on the installed version when they do, because the markers are still waiting. See forecastle#10 for why a second restart authority was refused rather than added. -- **A hard kill between arming the restart marker and completing the install - leaves both markers behind**, and the next start then boots the target. The - damage is bounded rather than absent: the target is unpacked with its - configuration materialised, so what happens is a cold boot of it rather than a - half-applied upgrade, and `releases/start_erl.data` still names the previous - permanent version, so the restart after that returns. Bounded is not closed. - - There is no test that plants the pair by hand, and that is a decision rather - than an omission. Both halves of the bound are already asserted by the restart - `:e2e` suite — the provisional boot of a `restart_emulator` target *is* a cold - boot of it, and killing it before the commit is the case that shows - `start_erl.data` bringing the previous version back. What a planted pair would - add is a state OTP's records contradict: forging the markers for a version the - handler has no `tmp_current` for leaves the node running one release while - `which_releases/0` reports another, and pinning that would be pinning an - incoherence rather than a property. If this is ever closed, the thing to change - is what the marker carries, not what a test plants. +- **A hard kill during a restart install can only lose the reboot, not misapply + it** — and where the pair survives such a kill, booting the target is right + rather than tolerated. Which window it lands in decides which: + + Between the arming and `prepare_restart_new_emulator/7`, only the marker + exists — OTP's file was cleared on the way in — so there is no pair, the next + start boots the permanent version, and the marker is consumed and discarded. + After `prepare_restart_new_emulator/7`, both exist, and so does a + `tmp_current` record for the target: the relup was evaluated in the VM that + died, the target is unpacked with its configuration materialised, and + `transform_release/3` will make it `current` on the way up. That is the state + the reboot was going to produce, so the next start producing it is the + protocol working. `releases/start_erl.data` still names the previous permanent + version either way, so nothing is committed by a crash. + + There is still no test that plants the pair by hand, and that is a decision + rather than an omission. What a planted pair adds is a state OTP's records + contradict — forging the markers for a version the handler has no + `tmp_current` for leaves the node running one release while `which_releases/0` + reports another — so pinning it would pin an incoherence. What *is* pinned is + the state each window leaves: a unit test observes the filesystem from inside + `install_release/1` and finds the marker alone, and the restart `:e2e` suite + covers the far window by killing the provisional release before the commit. - **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)). diff --git a/RELEASE.md b/RELEASE.md index 4334ba9..487617d 100644 --- a/RELEASE.md +++ b/RELEASE.md @@ -133,6 +133,22 @@ performed, because the alternative is a reboot that silently comes back on the version it was upgrading away from. + Agreeing on a version is not on its own enough, so the pair belongs to one + install *attempt* rather than to a version. Any `new_start_erl.data` left by an + earlier attempt is cleared before a new marker is armed - otherwise a retry of + the same version would arm a marker beside a file it did not write, and a + restart before the retry reached `:release_handler` would boot a version that + nothing had installed. A restart install while another one is already pending + is refused rather than allowed to take over its marker, saying so and changing + nothing; the marker is consumed by the next start of the deployment, so a + restart clears one left behind by an install that was interrupted. And the + marker records which attempt armed it, so a failed install removes only its own + - a start of the deployment consumes the marker whether or not it goes on to + boot, so the file at that path when an install fails is not necessarily the one + that install wrote. It is published by linking a file that is already complete + 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. + 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 diff --git a/lib/castle/commands.ex b/lib/castle/commands.ex index 76f615f..6657ff1 100644 --- a/lib/castle/commands.ex +++ b/lib/castle/commands.ex @@ -1,6 +1,15 @@ defmodule Castle.Commands do @moduledoc false + # `Castle.Peer` appears here twice over, and the two must not be conflated. + # `materialise/3` reaches it through a module *argument*, because what it does + # there is start a VM and a test has to be able to stand in for that. The + # restart marker uses the filesystem primitives on it directly - `work_dir/1`, + # `write_private/2`, `publish/2` - because those start nothing, and because + # what they guarantee is exactly what arming a marker needs. Do not inject + # them: a stub would prove nothing, and the guarantee is the point. + alias Castle.Peer + # A filesystem that reports no inode numbers has not said the two directories # differ; it has said nothing, which is a different answer. @no_inode "the filesystem reports no inode numbers, so the two cannot be compared" @@ -14,12 +23,27 @@ defmodule Castle.Commands do # `prepare_restart_new_emulator/7` writes that file *before* the reboot and # nothing ever removes it, so a preparation that failed after writing it - and # `transform_release/3` reconciles the release record without touching the - # marker - leaves a file naming a version that was never installed. The two - # together are the evidence: OTP's marker says which version, and this one says - # that Castle asked for the reboot that would boot it. The hook requires both, - # and requires them to name the same version. + # file - leaves one naming a version that was never installed. The two + # together are the evidence: OTP's file says which version, and this one says + # that Castle asked for the reboot that would boot it. + # + # **Agreeing on a version is not enough, and believing it was is what + # `arm_restart/4` had to be rewritten for.** Two files that merely name the + # same version say nothing about being the work of one install: a failed + # attempt to X leaves OTP's file naming X, a retry to X arms a fresh marker + # beside it, and a hard restart before the retry reaches `install_release/1` + # then presents a matching pair for an install that never happened - the node + # boots X with OTP's records calling it `unpacked`. So the pair has to belong + # to one *attempt*, which is `@provisional_marker` being cleared before the + # marker is armed, the marker being published exclusively, and the marker + # naming the attempt that wrote it. See `arm_restart/4`. @restart_marker "castle-restart-pending" + # OTP's half of the pair, written by `prepare_restart_new_emulator/7` and + # removed by nothing. Cleared before arming, which is what makes the pair + # evidence about one attempt rather than about a version. + @provisional_marker "new_start_erl.data" + # The two instructions `release_handler` treats as "reboot the emulator", and # what `do_install_release/3` does with each. Only the one-stage instruction is # a transition the launcher can select the target of; see `restart_planned?/3`. @@ -422,7 +446,7 @@ defmodule Castle.Commands do about why. """ @spec materialise(Path.t(), module(), module()) :: result() - def materialise(rel_vsn_dir, peer \\ Castle.Peer, deployment \\ Castle.Deployment) do + def materialise(rel_vsn_dir, peer \\ Peer, deployment \\ Castle.Deployment) do vsn = Path.basename(rel_vsn_dir) with :ok <- ensure_own_erts("Cannot configure #{vsn}", deployment) do @@ -511,6 +535,11 @@ defmodule Castle.Commands do `rel_dir` is where the restart marker is armed. It is an argument for the reason `make_releases/3`'s is: nothing chooses it, `Castle.install/1` derives it from `code:root_dir()`, and a test needs somewhere to look. + + A transition that reboots the emulator is refused, with nothing touched, while + another such install is still pending - see `arm_restart/4`. One at a time is + the price of the marker being evidence about a particular install rather than + about a version. """ @spec install(String.t(), Path.t(), module(), module()) :: result() def install(vsn, rel_dir, handler \\ :release_handler, deployment \\ Castle.Deployment) do @@ -531,8 +560,8 @@ defmodule Castle.Commands do restart? = restart_planned?(vsn, rel_dir, running) with :ok <- refuse_synthesised(refusal, running), - :ok <- arm_restart(restart?, vsn, rel_dir, refusal) do - installed(vsn, restart?, rel_dir, handler) + {:ok, attempt} <- arm_restart(restart?, vsn, rel_dir, refusal) do + installed(vsn, attempt, rel_dir, handler) end end @@ -541,9 +570,9 @@ defmodule Castle.Commands do # which is why the transition is classified from the relup beforehand and the # answer carried in here. Reporting a reboot as "now running" is a claim that is # false for as long as the reboot takes, and automation reads it. - defp installed(vsn, restart?, rel_dir, handler) do + defp installed(vsn, attempt, rel_dir, handler) do case handler.install_release(to_charlist(vsn)) do - {:ok, other_vsn, _descr} when restart? -> + {:ok, other_vsn, _descr} when is_binary(attempt) -> {:ok, [ "Installed #{vsn} (previously #{other_vsn}). The emulator is restarting.", @@ -567,11 +596,11 @@ defmodule Castle.Commands do ]} {:error, reason} -> - disarm(restart?, rel_dir) + disarm(attempt, rel_dir) {:error, "Install of #{vsn} failed. #{inspect(reason)}"} other -> - disarm(restart?, rel_dir) + disarm(attempt, rel_dir) {:error, "Install of #{vsn} returned an unexpected result. #{inspect(other)}"} end end @@ -663,41 +692,233 @@ defmodule Castle.Commands do end # Armed before `install_release/1` is asked for anything, so that the marker is - # in place before OTP writes its own and reboots. A failure to write it refuses - # the install rather than going ahead: the reboot would come back on whichever - # version `releases/start_erl.data` names, which is the one being upgraded - # away from, and the upgrade would be lost with nothing saying so. - defp arm_restart(false, _vsn, _rel_dir, _refusal), do: :ok + # in place before OTP writes its own and reboots, and armed as **this + # attempt's** rather than as the version's. Three steps, in this order, and the + # order is the protocol: + # + # 1. **Refuse if a marker is already there.** One pending restart install at + # a time. Two attempts sharing one name overwrite and disarm each other, + # and the survivor's marker says nothing about which of them - if either - + # reached `install_release/1`. `publish/2` is what actually decides this, + # by refusing rather than replacing; the look first is what keeps step 2 + # from destroying a pending attempt's evidence, since anything in flight + # holds the marker from before its own step 2 until its `disarm/2`. A + # marker that appears between the look and the publish belongs to an + # attempt that has not reached `install_release/1` yet, so there is + # nothing of its to destroy. + # 2. **Clear OTP's file.** `prepare_restart_new_emulator/7` writes + # `releases/new_start_erl.data` and nothing removes it, so one left by an + # earlier failure would pair with the marker armed next and boot a version + # this attempt never installed. Removing it is safe: `write_new_start_erl/3` + # goes through `file:write_file/2`, which creates the file when it is + # absent. After this, that file existing means *this* attempt's + # preparation wrote it. + # 3. **Publish the marker.** Staged in an owner-only working directory and + # hard-linked into place, which is how `Castle.Peer` publishes + # `sys.config.pristine` and for the same two reasons: a link publishes a + # file that is already complete, so no start can read a marker that is + # empty or half written, and it refuses rather than replaces, so the loser + # of a race is told instead of silently taking the marker over. An + # exclusive create in place would have neither property - it makes + # *creation* atomic and leaves the file empty between the open and the + # write, and a death in that window leaves an empty marker that blocks + # every later attempt. + # + # Steps 1 and 2 are in that order and must stay in it. Reversed, an attempt + # would refuse *after* clearing OTP's file, which is how a concurrent install + # loses its reboot silently. + # + # A failure at any step refuses the install rather than going ahead: the reboot + # would come back on whichever version `releases/start_erl.data` names, which + # is the one being upgraded away from, and the upgrade would be lost with + # nothing saying so. + defp arm_restart(false, _vsn, _rel_dir, _refusal), do: {:ok, nil} defp arm_restart(true, vsn, rel_dir, refusal) do + attempt = attempt() + + with :ok <- unclaimed(rel_dir, refusal), + :ok <- clear_provisional(rel_dir, vsn, refusal), + :ok <- publish_marker(rel_dir, vsn, attempt, refusal) do + {:ok, attempt} + end + end + + # What names this attempt, and the whole of what `disarm/2`'s ownership rests + # on. The operating system pid, the wall clock in nanoseconds and a number no + # other call in this VM will use again: unique within a node, and unique across + # a node's restarts, which is as far as ownership has to reach - the marker + # never outlives the deployment's next start, because the hook consumes it. + # + # It is not a secret and does not need to be. Anything able to forge it can + # write in the releases directory, where it could write the marker itself. + # What it defends against is *confusion*: the marker's name is shared, so + # removing "the marker" is not the same as removing the one this attempt + # published. + # + # The shell side never reads it. The version is the first line, which is what + # `head -n 1` gives the hook, and everything after it is Castle's own + # bookkeeping - so the file can carry this without the hook having to parse + # anything it did not before. + defp attempt do + serial = System.unique_integer([:monotonic, :positive]) + + "#{System.pid()}-#{System.system_time(:nanosecond)}-#{serial}" + end + + # Step 1. `lstat` rather than `File.exists?/1`, because the three answers want + # three different things said: a regular file is a pending attempt and names + # its version, anything else is a name in use by something that is not a + # marker, and a lookup that failed is neither and says so. + defp unclaimed(rel_dir, refusal) do marker = Path.join(rel_dir, @restart_marker) - case File.write(marker, vsn <> "\n") do - :ok -> - :ok + case File.lstat(marker) do + {:error, :enoent} -> :ok + {:ok, %File.Stat{type: :regular}} -> {:error, pending(marker, refusal)} + {:ok, %File.Stat{type: type}} -> {:error, occupied(marker, type, refusal)} + {:error, reason} -> {:error, unarmed(marker, reason, refusal)} + end + end + + # Step 2. A file that is not there is the ordinary case and not an error; one + # that will not go is refused, because going on would leave this attempt's + # marker pairable with an earlier attempt's file. + defp clear_provisional(rel_dir, vsn, refusal) do + provisional = Path.join(rel_dir, @provisional_marker) + + case File.rm(provisional) do + :ok -> :ok + {:error, :enoent} -> :ok + {:error, reason} -> {:error, stale(provisional, vsn, reason, refusal)} + end + end + + # Step 3. The working directory is removed on every way out, and only ever the + # one this call made - the rule `Castle.Peer` follows, and for the reason it + # gives: staging that never got published cannot be told from another install's + # work in progress, so nothing goes looking for it. + defp publish_marker(rel_dir, vsn, attempt, refusal) do + marker = Path.join(rel_dir, @restart_marker) + + case Peer.work_dir(rel_dir) do + {:ok, work} -> + outcome = staged(Path.join(work, @restart_marker), marker, "#{vsn}\n#{attempt}\n") + File.rm_rf(work) + armed(outcome, marker, refusal) {:error, reason} -> - {:error, - "#{refusal}: the upgrade to #{vsn} restarts the emulator, and #{marker} - " <> - "which is what tells the launcher to boot #{vsn} when the system comes back - " <> - "could not be written (#{:file.format_error(reason)}). Without it the restart " <> - "would come back on the version releases/start_erl.data names, losing the " <> - "upgrade."} + {:error, unarmed(marker, reason, refusal)} end end + defp staged(staging, marker, bytes) do + with :ok <- Peer.write_private(staging, bytes), do: Peer.publish(staging, marker) + end + + defp armed(:ok, _marker, _refusal), do: :ok + defp armed(:taken, marker, refusal), do: {:error, pending(marker, refusal)} + defp armed({:error, reason}, marker, refusal), do: {:error, unarmed(marker, reason, refusal)} + # Cleared on every path out of a failed install, because a marker left armed # beside a `new_start_erl.data` the same failure may already have written is # exactly the pair the hook acts on. # - # The result is deliberately not looked at, and that is not laziness: a - # releases directory this cannot remove the marker from is one `arm_restart/4` - # could not have written it into, and that refuses the install before - # `install_release/1` is asked for anything. So reaching here at all means the - # marker was writable moments ago, and there is no state left worth branching - # on - only a state nothing could produce and no test could arrange. - defp disarm(false, _rel_dir), do: :ok - defp disarm(true, rel_dir), do: File.rm(Path.join(rel_dir, @restart_marker)) + # **Only if this attempt is the one that armed it.** The marker's name is + # shared and the marker itself is short-lived: any `start` or `daemon` of the + # deployment consumes it, whether or not that start went on to boot, so a + # marker at this path by the time an install fails is not necessarily the one + # the install published. Removing it by name would take a later attempt's + # marker away and lose that attempt's reboot. So the attempt is read back out + # of the file and compared, and a marker that says something else is left where + # it is. + # + # The check and the removal are two calls, so a marker consumed and re-armed + # between them is still removed. That window is two adjacent statements wide + # and there is no POSIX operation that closes it - unlinking is by name, and no + # name carries its identity. What it costs if it is ever hit is a lost reboot, + # which is the direction the whole protocol fails in. + # + # The result of the removal is deliberately not looked at: a releases directory + # this cannot remove the marker from is one `publish_marker/4` could not have + # linked it into, and that refuses the install before `install_release/1` is + # asked for anything. + defp disarm(nil, _rel_dir), do: :ok + + defp disarm(attempt, rel_dir) do + marker = Path.join(rel_dir, @restart_marker) + + if armed_by?(marker, attempt) do + File.rm(marker) + end + + :ok + end + + defp armed_by?(marker, attempt) do + case File.read(marker) do + {:ok, contents} -> match?([_vsn, ^attempt | _], String.split(contents, "\n")) + {:error, _unreadable} -> false + end + end + + ## What each way of failing to arm says + + defp pending(marker, refusal) do + "#{refusal}: #{marker} is already there, so a restart install is pending - " <> + "#{armed_version(marker)}. Two of them cannot share that file: the second " <> + "would overwrite the first, and whichever marker survived would say nothing " <> + "about which install reached :release_handler. Nothing has been changed. The " <> + "marker is consumed by the next start of this deployment, so a restart clears " <> + "it; if no install is in flight and the system is not going to be restarted, " <> + "remove that file." + end + + defp armed_version(marker) do + case File.read(marker) do + {:ok, contents} -> named(contents |> String.split("\n") |> hd()) + {:error, reason} -> "it cannot be read (#{:file.format_error(reason)})" + end + end + + # A marker Castle published always has a version on its first line, because it + # is linked into place complete. One without is somebody else's file under + # Castle's name, and saying so is more use than a message with a gap in it. + defp named(""), do: "it names no version" + defp named(vsn), do: "it names #{vsn}" + + defp occupied(marker, type, refusal) do + "#{refusal}: #{marker} is where Castle records that an upgrade asked for a " <> + "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." + end + + defp describe_type(:directory), do: "a directory" + defp describe_type(:symlink), do: "a symbolic link" + defp describe_type(other), do: "a #{other}" + + defp stale(provisional, vsn, reason, refusal) do + "#{refusal}: the upgrade to #{vsn} restarts the emulator, and #{provisional} - " <> + "which :release_handler writes before the reboot and never removes - could not " <> + "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." + end + + defp unarmed(marker, reason, refusal) do + "#{refusal}: the upgrade restarts the emulator, and #{marker} - which is what " <> + "tells the launcher which version to boot when the system comes back - could " <> + "not be armed: #{detail(reason)}. Without it the restart would come back on " <> + "the version releases/start_erl.data names, losing the upgrade." + end + + # A `:file` reason from the `lstat`, or a message from one of the primitives in + # `Castle.Peer`, which have already said which path and why. + defp detail(reason) when is_atom(reason), do: to_string(:file.format_error(reason)) + defp detail(message) when is_binary(message), do: String.trim_trailing(message, ".") @doc """ Confirms that `vsn` is the release the system is running. diff --git a/test/castle/commands_test.exs b/test/castle/commands_test.exs index 3ebf2d4..7de8c0d 100644 --- a/test/castle/commands_test.exs +++ b/test/castle/commands_test.exs @@ -240,7 +240,16 @@ defmodule Castle.CommandsTest do handler = real_record(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) assert {:ok, lines} = Commands.install("1.2.3", dir, handler) - assert File.read!(marker(dir)) == "1.2.3\n" + + # The version on the first line, which is all the hook reads, and the + # attempt on the second, which is what makes the file this install's rather + # than this version's. Asserted as two lines rather than as a whole string + # so that the attempt stays opaque here: what it is made of is + # `Castle.Commands.attempt/0`'s business, and the only property this suite + # rests on is that it is there and that it differs between attempts. + assert [vsn, attempt] = File.read!(marker(dir)) |> String.split("\n", trim: true) + assert vsn == "1.2.3" + assert attempt != "" # And the report says what happened rather than what a hot upgrade's reply # would have suggested: the same {ok, ...} means the emulator is going down. @@ -262,7 +271,7 @@ defmodule Castle.CommandsTest do handler = real_record(:install_release, {:ok, ~c"1.2.2", ~c"downgrade"}) assert {:ok, _} = Commands.install("1.2.1", dir, handler) - assert File.read!(marker(dir)) == "1.2.1\n" + assert armed_version(dir) == "1.2.1" end @tag :tmp_dir @@ -327,21 +336,39 @@ defmodule Castle.CommandsTest do end @tag :tmp_dir - test "refuses the install when it cannot be armed", %{tmp_dir: dir} do - # A directory at the marker's name is what makes this deterministic; what it - # stands for is a releases directory that cannot be written to. Going ahead - # would reboot the system and come back on the version start_erl.data names, - # losing the upgrade with nothing saying so - so the install is refused - # before install_release/1 is asked for anything, which is the line - # everything else here is on the right side of too. + test "refuses the install when something else holds the name", %{tmp_dir: dir} do + # A directory at the marker's name is what makes this deterministic; a + # fixture here may not turn on a mode that root or a filesystem can ignore. + # Going ahead would reboot the system and come back on the version + # start_erl.data names, losing the upgrade with nothing saying so - so the + # install is refused before install_release/1 is asked for anything, which + # is the line everything else here is on the right side of too. relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) File.mkdir!(marker(dir)) handler = real_record(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) + assert {:error, message} = Commands.install("1.2.3", dir, handler) + assert message =~ "Cannot install 1.2.3: #{marker(dir)}" + assert message =~ "there is already a directory at that path" + assert Stub.calls(:install_release) == [] + end + + @tag :tmp_dir + test "refuses the install when it cannot be armed", %{tmp_dir: dir} do + # The other half of that: the marker's name is free, and OTP's file - which + # has to be cleared before the marker is armed - cannot be got rid of. A + # directory at *its* name is the deterministic stand-in for a releases + # directory nothing may write to, and the refusal has to come before + # install_release/1 for the same reason. + relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) + File.mkdir!(provisional(dir)) + handler = real_record(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) + assert {:error, message} = Commands.install("1.2.3", dir, handler) assert message =~ "Cannot install 1.2.3: the upgrade to 1.2.3 restarts the emulator" - assert message =~ Path.join(dir, "castle-restart-pending") - assert message =~ "losing the upgrade" + assert message =~ provisional(dir) + assert message =~ "would pair with the marker this install is about to arm" + refute File.exists?(marker(dir)) assert Stub.calls(:install_release) == [] end @@ -353,6 +380,122 @@ defmodule Castle.CommandsTest do assert {:error, _} = Commands.install("1.2.3", dir, handler, erts_less()) refute File.exists?(marker(dir)) end + + @tag :tmp_dir + test "leaves nothing of its own in the releases directory", %{tmp_dir: dir} do + # The marker is staged in a working directory of its own and hard-linked + # into place - the way `Castle.Peer` publishes `sys.config.pristine`, and + # for the same reasons: a link publishes a file that is already complete, + # and refuses rather than replaces. What that must not leave behind is the + # staging, so the only `castle-` name here afterwards is the marker itself. + relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) + handler = real_record(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) + + assert {:ok, _} = Commands.install("1.2.3", dir, handler) + assert Path.wildcard(Path.join(dir, "castle-*")) == [marker(dir)] + end + end + + # Two files that merely agree on a version are not evidence that one install + # produced them. `new_start_erl.data` is written before the reboot and removed + # by nothing, so a failed attempt leaves OTP's half of the pair behind, and a + # retry to the same version used to arm a fresh marker beside it - after which a + # restart before `install_release/1` was reached presented a matching pair for + # an install that never happened. + # + # What makes the pair an attempt's rather than a version's: OTP's file is + # cleared before the marker is armed, the marker is published exclusively so two + # attempts cannot share it, and the marker names the attempt that wrote it so + # that no attempt removes another's. + describe "the restart marker, as this attempt's" do + @tag :tmp_dir + test "clears the file a failed attempt left before arming", %{tmp_dir: dir} do + # The reviewer's sequence, run: an attempt to 1.2.3 gets as far as OTP + # writing new_start_erl.data and then fails, and the same version is + # installed again. The retry has to leave that file gone, because until it + # is written afresh there is no pair for a restart to act on. + relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) + handler = real_record(:install_release, prepares_then_fails(dir, "1.2.3")) + + assert {:error, _} = Commands.install("1.2.3", dir, handler) + refute File.exists?(marker(dir)), "the marker survived a failed install" + assert File.exists?(provisional(dir)), "the fixture did not leave OTP's file behind" + + # The retry, which must not be able to pair with what the first left. + Stub.stub(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) + + assert {:ok, _} = Commands.install("1.2.3", dir, handler) + assert armed_version(dir) == "1.2.3" + + refute File.exists?(provisional(dir)), + "a stale new_start_erl.data survived the arming that came after it" + end + + @tag :tmp_dir + test "is alone on disk until install_release/1 has written OTP's half", + %{tmp_dir: dir} do + # What a hard restart between the arming and the reboot finds, observed + # from inside the only call that happens between them. A marker with no + # `new_start_erl.data` beside it is not a pair, so the hook does nothing + # with it and the system comes back on the permanent version - which is the + # direction this protocol is built to fail in. The state is unobservable + # from outside the call, because a successful install leaves the marker + # armed either way. + relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) + File.write!(provisional(dir), "16.0 1.2.3") + + handler = + real_record(:install_release, fn _args -> + send(self(), {:when_asked, File.exists?(marker(dir)), File.exists?(provisional(dir))}) + {:ok, ~c"1.2.2", ~c"upgrade"} + end) + + assert {:ok, _} = Commands.install("1.2.3", dir, handler) + assert_received {:when_asked, true, false} + end + + @tag :tmp_dir + test "refuses while another restart install is pending", %{tmp_dir: dir} do + # One at a time. Two attempts sharing the marker overwrite and disarm each + # other, and the survivor's marker says nothing about which of them reached + # :release_handler. The install is refused with nothing touched - notably + # not the pending marker, and not OTP's file, which the refusal has to fall + # in front of rather than after. + relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) + File.write!(marker(dir), "1.2.4\nsomeone-elses-attempt\n") + File.write!(provisional(dir), "16.0 1.2.4") + handler = real_record(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) + + assert {:error, message} = Commands.install("1.2.3", dir, handler) + assert message =~ "a restart install is pending - it names 1.2.4" + assert message =~ "consumed by the next start of this deployment" + assert File.read!(marker(dir)) == "1.2.4\nsomeone-elses-attempt\n" + assert File.exists?(provisional(dir)) + assert Stub.calls(:install_release) == [] + end + + @tag :tmp_dir + test "does not remove a marker it did not arm", %{tmp_dir: dir} do + # Any start of the deployment consumes the marker, whether or not it goes on + # to boot, so a marker at that path when an install fails is not + # necessarily the one that install armed. Removing it by name would take a + # later attempt's reboot away. Arranged by replacing the marker from inside + # install_release/1, which is the only moment between the arming and the + # disarming. + relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) + + foreign = "1.2.4\nsomeone-elses-attempt\n" + + handler = + real_record(:install_release, fn _args -> + File.rm!(marker(dir)) + File.write!(marker(dir), foreign) + {:error, :whatever} + end) + + assert {:error, _} = Commands.install("1.2.3", dir, handler) + assert File.read!(marker(dir)) == foreign + end end describe "running/3" do @@ -705,6 +848,25 @@ defmodule Castle.CommandsTest do defp unpacked(dir), do: File.write!(Path.join(dir, "sys.config"), "[].\n") defp marker(rel_dir), do: Path.join(rel_dir, "castle-restart-pending") + defp provisional(rel_dir), do: Path.join(rel_dir, "new_start_erl.data") + + # The version the marker names, which is its first line and the only part the + # launcher's hook reads. + defp armed_version(rel_dir) do + rel_dir |> marker() |> File.read!() |> String.split("\n") |> hd() + end + + # An `install_release/1` that does what `prepare_restart_new_emulator/7` does + # before it fails: writes `new_start_erl.data` naming the target, and then + # errors. That is the state no end-state fixture can produce, because the file + # only comes into being while the call is in flight - and it is the state that + # made the pair correlate by version rather than by attempt. + defp prepares_then_fails(rel_dir, vsn) do + fn _args -> + File.write!(provisional(rel_dir), "16.0 #{vsn}") + {:error, {:bad_relup_file, ~c"relup"}} + end + end # A relup where `release_handler` reads one: `releases//relup`, holding a # single `{Vsn, Ups, Downs}` term. Written as a term rather than as text so that diff --git a/test/support/release_handler_stub.ex b/test/support/release_handler_stub.ex index 6f298b5..052b78b 100644 --- a/test/support/release_handler_stub.ex +++ b/test/support/release_handler_stub.ex @@ -12,6 +12,13 @@ defmodule Castle.ReleaseHandlerStub do @doc """ Registers the value the named function replies with, and returns this module so that it can be passed straight to the function under test. + + A reply that is a function of one argument is called with the call's arguments + and its result used as the reply. That is for the tests about *ordering* around + a mutating call: `install_release/1` is the one thing that happens between + arming the restart marker and reporting, and `prepare_restart_new_emulator/7` + writing `new_start_erl.data` before it can still fail is a state no end-state + fixture can produce, because it only exists while a call is in flight. """ def stub(fun, reply) when is_atom(fun) do Process.put({__MODULE__, fun}, reply) @@ -45,6 +52,7 @@ defmodule Castle.ReleaseHandlerStub do case Process.get({__MODULE__, fun}, :unstubbed) do :unstubbed -> raise "#{fun}/#{length(args)} was called without a registered reply" + reply when is_function(reply, 1) -> reply.(args) reply -> reply end end From 6ea1fc2e1616b007785ea072771d05f86ef7fb19 Mon Sep 17 00:00:00 2001 From: ausimian Date: Sun, 23 Aug 2026 16:30:22 +1000 Subject: [PATCH 3/6] fix: serialise the whole install, not just the marker's arming arm_restart/4's three steps are one caller's sequence, and the argument that release_handler serialises install_release/1 anyway does not carry them: that serialisation is downstream of the entire protocol. Two callers read the running release, classify the transition and pass the marker check before either of them publishes anything, so refusing before clearing buys nothing across processes. The loser reaches clear_provisional/3 after the winner's install_release/1 has written new_start_erl.data, deletes the winner's live evidence, and refuses - the winner's reboot then comes back on the permanent release, install waits for a version that never becomes the running one, and the operator is told that nothing has been changed by the process that changed it. The protocol is not reordered to avoid that, because publishing before clearing would leave the marker pairable with a stale new_start_erl.data and boot a version nothing installed, which is worse. What is added is that there is only ever one caller in it: the running-release read, the classification, the arming, install_release/1 and the disarming all run inside :global.trans/3 over [node()]. The region has to reach that far for a second reason - restart_planned?/3 is a prediction about the running release, so a concurrent hot install moves the from-version and do_get_rh_script/4 then selects a different relup entry from the one that was classified. global_name_server is a kernel process and runs whether or not distribution does, and a lock restricted to [node()] talks to the local one only, so this works on a node with is_alive() == false - which is the ordinary case here, and the case it was measured on. trans/3 releases the lock in an after and global monitors the holder, so a caller that dies does not wedge every later install; retries are infinity, so there is no aborted to mean anything by. It needs no process of Castle's own, which a supervised lock server would have added to the managed system's supervision tree for a command that runs a handful of times in a deployment's life. [node()] rather than [node() | nodes()] because every caller arrives by rpc on the running node: a cluster-wide lock would wait on nodes that share nothing with the deployment and would still not cover a caller in another VM, which is the boundary and is documented as one. It waits rather than refusing, and the waiter then meets the marker check and 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 the lock: unpack, commit and remove arm nothing, and the materialisation Castle.install/1 does first stays outside it. The tests drive two real callers with a barrier at which_releases/0, which is the first thing inside the region and the last before the arming - so a caller held there is one that has taken the region and armed nothing, which is the state two of them could previously occupy at once. The discriminator is that the second caller's lookup never happens; the end state is deliberately not one, because the interleaving that destroys evidence needs a caller suspended between the marker check and the clearing, and those are adjacent. A third test installs one relup concurrently from two different running versions, where the same target is hot from one and a restart from the other, so the classification is shown to belong to the caller that made it. Refs: https://github.com/ausimian/forecastle/issues/10 Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN --- AGENTS.md | 129 ++++++++++++++++++++++--- RELEASE.md | 14 ++- lib/castle/commands.ex | 135 +++++++++++++++++++++++--- test/castle/commands_test.exs | 174 +++++++++++++++++++++++++++++++++- 4 files changed, 422 insertions(+), 30 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index fbea156..4aca07d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -602,20 +602,21 @@ Castle's job is configuration and release management on a running node. Back-to-back or concurrent installs broke it the other way, by overwriting and disarming each other's marker. - **So the pair is owned by an install *attempt*, and three things make it so.** - `arm_restart/4` is the whole of it and the order is the protocol. + **So the pair is owned by an install *attempt*, and four things make it so.** + `arm_restart/4` is three of them and the order is the protocol; the fourth is + that there is only ever one caller in the install at all. 1. **One pending restart install at a time.** A marker already at the path - refuses the install rather than being adopted or replaced. `publish/2` - decides it, by refusing rather than replacing; the `lstat` first is what - keeps step 2 from destroying a pending attempt's evidence, since anything in - flight holds the marker from before its own step 2 until its `disarm/2`. + refuses the install rather than being adopted or replaced, which is what + keeps step 2 from clearing the `new_start_erl.data` that attempt's + preparation wrote. `publish/2` decides it a second time over, by refusing + rather than replacing. 2. **OTP's file is cleared before the marker is armed.** That is what closes the window above: after it, `new_start_erl.data` existing means *this* attempt's preparation wrote it. Removing it is safe — `write_new_start_erl/3` goes through `file:write_file/2`, which creates the file when it is absent. The order matters and must not be reversed: an - attempt that refused *after* clearing would take a concurrent install's + attempt that refused *after* clearing would take an already-requested reboot away silently. 3. **The marker names the attempt that armed it**, on a second line, and `disarm/2` removes it only if it still does. The marker's name is shared and @@ -630,6 +631,72 @@ Castle's job is configuration and release management on a running node. confusion, not forgery. **The hook never reads it**: the version is the first line, which is what `head -n 1` gives it, so the file carries this without the shell parsing anything it did not before. + 4. **One caller in the install at a time**, which is `Castle.Commands.serialised/2` + and the whole of what makes the first three mean anything across processes. + + **Steps 1 to 3 are one caller's sequence, and the first version of this + believed that `release_handler` serialising `install_release/1` was enough to + make them a protocol. It is not: that serialisation is *downstream* of all of + them.** Two callers both read the running release, both classify it and both + pass step 1, because none of that has published anything yet. Step 1 reversed + with step 2 is then no protection at all — the loser reaches step 2 *after* the + winner's `install_release/1` has written `new_start_erl.data`, deletes it, and + the winner's reboot comes back on the permanent release while the loser reports + that nothing has been changed. An operator sees a timeout and a false + reassurance. + + **Do not answer this by reordering the protocol.** Publishing before clearing + leaves a window in which the marker pairs with a *stale* `new_start_erl.data`, + and the hook then boots a version nothing installed — which is worse than + losing a reboot, and is what step 2's position exists to prevent. The order is + right for one caller; the fix is that there is one caller. + + **The serialised region is the whole install, not just the arming**, because the + classification is a prediction about the running release: an install that + completes between `restart_planned?/3` and `install_release/1` moves the + from-version, so `do_get_rh_script/4` evaluates a different relup entry and the + armed state disagrees with the transition OTP selects. So the running-release + read, the classification, the arming, `install_release/1` and the disarming are + all inside it. The ERTS guard is the one part deliberately outside — it reads + two directories and refuses without touching anything, and a refusal has no + reason to wait. + + **It is `:global.trans/3` over `[node()]`, and the mechanism was chosen rather + than assumed.** `global_name_server` is a kernel process running whether or not + distribution is, and `set_lock/2` restricted to `[node()]` talks to the local + one only — so this works on a node with `is_alive() == false`, which is the + ordinary case for a release that configures no distribution, and is the case it + was measured on. `trans/3` releases the lock in an `after` and `global` monitors + the holder besides, so a caller that dies releases it instead of wedging every + later install; retries are `infinity`, so there is no `aborted` to have to mean + something by. The alternative was a process of Castle's own, and it is a worse + trade: these modules are deliberately stateless and run inline in whatever + process asked, so a lock server would be a new entry in the *managed* system's + supervision tree, with a lifetime and a restart strategy, to serialise a command + that runs a handful of times in a deployment's life. + + `[node()]` rather than the default `[node() | nodes()]` because every caller + arrives on the running node — `bin/castle` is `rpc`, and the launcher's preboot + only calls `make_releases/0` — so this node is the whole set of callers. A + cluster-wide lock would make an install wait on nodes that share nothing with + the deployment, and make a network partition its business, and it still would + not cover a caller in some other VM. **That is the boundary and it should be + said plainly:** a second VM writing into this releases directory is outside the + lock, and what defends the marker there is the filesystem half alone — + `publish/2` refusing rather than replacing — which is no worse than before and + no better. It waits rather than refusing, and the waiter then meets step 1 and + 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. The + materialisation `Castle.install/1` does *first* is outside it too, and + deliberately: it writes into the target's own version directory and touches no + release record, its own primitives already refuse rather than replace, and + holding this lock across a peer VM's boot would put every install behind + another's configuration step. Nothing about the marker protocol depends on it. **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 @@ -944,11 +1011,36 @@ arming and the disarming, so that `disarm/2` can be shown to leave a marker it did not write. None of those has an end state that distinguishes it — a successful install leaves the marker armed either way, and a failed one leaves it gone either way — which is the same reason `Castle.Peer`'s primitives are public. -The concurrency case needs no seam: a marker already at the path is a pending -attempt, and what is asserted is that the install is refused, that -`install_release/1` was never called, and that *neither* the marker nor OTP's file -was touched — the second half being the ordering that keeps a refusal from -destroying a concurrent install's evidence. +A marker already at the path needs no seam of its own: it is a pending attempt, +and what is asserted is that the install is refused, that `install_release/1` was +never called, and that *neither* the marker nor OTP's file was touched — the +second half being the ordering that keeps a refusal from destroying a pending +attempt's evidence. + +**Two real callers do need one, and the seam is `which_releases/0` rather than +`install_release/1`.** The state that used to be reachable is two callers past the +running-release lookup and neither of them armed, so what has to be held open is +the *front* of the serialised region — and the lookup is both the first thing in +it and the last thing before the marker is armed. So `installer/3` runs +`install/4` in a task of its own with a `which_releases/0` that reports where it +got to and, optionally, waits to be released; a second caller is started while the +first is held there, and the discriminator is that its lookup never happens +(`refute_receive {:looked_up, :second}`). That is the only kind of assertion +available, and it is honest about why: the end state is the *same* either way — +the second caller is refused for a marker it finds and changes nothing — because +the interleaving that destroys evidence needs a caller suspended between step 1 +and step 2, which has no seam and needs none once there cannot be two. +`Task.await` and `assert_receive` carry generous timeouts because `global`'s lock +retry backs off by up to a second or two, and every stub reply and call record +lives in the *task's* process dictionary, so a caller answers with its own +`Stub.calls(:install_release)` rather than the test reading them. + +Three of them, and the third is not about the marker: one relup for 1.2.3 whose +transition from 1.2.2 is hot and from 1.2.1 restarts the emulator, installed +concurrently by two callers running different versions. It says the classification +belongs to the caller that made it — which is the second half of why the region +reaches past the arming — and that the hot caller neither adopts nor disarms the +marker the restarting one is waiting on. What is *not* covered here is a booted release: the upgrade of a running system, and the exit statuses `bin/castle` returns, belong to Forecastle's @@ -1007,6 +1099,19 @@ wrote, so Elixir's pipeline is still armed in the file the launcher reads. the state each window leaves: a unit test observes the filesystem from inside `install_release/1` and finds the marker alone, and the restart `:e2e` suite covers the far window by killing the provisional release before the commit. +- **Serialising the install is node-local, so a second VM writing into the same + releases directory is outside it.** `Castle.Commands.serialised/2` locks over + `[node()]`, which is exact for every caller Castle has — `bin/castle` is `rpc`, + and the launcher's preboot step calls only `make_releases/0` — but it is a + statement about callers rather than about the directory. Something else running + `Castle.install/1` in a VM of its own against the same deployment gets the + filesystem half of the protocol and nothing more: `publish/2` refuses rather + than replaces, so the marker cannot be silently taken over, and the window the + lock closes — two callers both past `unclaimed/2` before either publishes — is + 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. - **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)). diff --git a/RELEASE.md b/RELEASE.md index 487617d..8db957d 100644 --- a/RELEASE.md +++ b/RELEASE.md @@ -138,7 +138,19 @@ earlier attempt is cleared before a new marker is armed - otherwise a retry of the same version would arm a marker beside a file it did not write, and a restart before the retry reached `:release_handler` would boot a version that - nothing had installed. A restart install while another one is already pending + nothing had installed. + + Only one install runs on the node at a time, and that is what makes the + clearing mean anything: two of them could otherwise both decide to arm before + 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. + + A restart install while another one is already pending is refused rather than allowed to take over its marker, saying so and changing nothing; the marker is consumed by the next start of the deployment, so a restart clears one left behind by an install that was interrupted. And the diff --git a/lib/castle/commands.ex b/lib/castle/commands.ex index 6657ff1..4609350 100644 --- a/lib/castle/commands.ex +++ b/lib/castle/commands.ex @@ -37,6 +37,12 @@ defmodule Castle.Commands do # to one *attempt*, which is `@provisional_marker` being cleared before the # marker is armed, the marker being published exclusively, and the marker # naming the attempt that wrote it. See `arm_restart/4`. + # + # Those three are about one caller's sequence, and they are not on their own + # enough either: two callers can run the sequence at once, and then the loser + # clears the winner's `@provisional_marker` after the winner has written it. + # So the whole of the install - including the read and the classification in + # front of it - is serialised on this node. See `serialised/2`. @restart_marker "castle-restart-pending" # OTP's half of the pair, written by `prepare_restart_new_emulator/7` and @@ -540,20 +546,106 @@ defmodule Castle.Commands do another such install is still pending - see `arm_restart/4`. One at a time is the price of the marker being evidence about a particular install rather than about a version. + + Two installs cannot be under way on this node at once at all - see + `serialised/2`. The refusal above is what the second one is then told, once the + first has finished and its marker is complete. """ @spec install(String.t(), Path.t(), module(), module()) :: result() def install(vsn, rel_dir, handler \\ :release_handler, deployment \\ Castle.Deployment) do with :ok <- ensure_own_erts("Cannot install #{vsn}", deployment) do - install_upgradable(vsn, rel_dir, handler) + serialised(rel_dir, fn -> install_upgradable(vsn, rel_dir, handler) end) end end - # Everything after the ERTS guard, with the running release asked for once. + ## One install at a time + + # The resource two callers contend for: this module's install of this + # deployment. `rel_dir` is in it because that is the deployment - there is one + # per node, so it changes nothing in a release, and it is what lets the unit + # suite stay async, each test contending only for its own `tmp_dir`. + @install_lock {__MODULE__, :install} + + # Everything after the ERTS guard, run with no other caller in it. + # + # **The three steps of `arm_restart/4` are correct for one caller and say + # nothing across processes, and `release_handler` serialising `install_release/1` + # does not close that.** Its serialisation is *downstream* of the whole + # protocol: two callers can both read the running release, both classify it, + # and both pass `unclaimed/2`, because all of that happens before either of + # them publishes anything. Refusing before clearing then buys nothing. The + # loser's `clear_provisional/3` runs after the winner's `install_release/1` + # has written `new_start_erl.data`, so it deletes the winner's live evidence; + # the winner's reboot comes back on the permanent release, `install` waits for + # a version that never becomes the running one, and the operator is told + # "Nothing has been changed" by the process that changed it. + # + # **Do not reorder the protocol to avoid that.** Publishing before clearing + # leaves a window in which the marker pairs with a *stale* `new_start_erl.data`, + # and the hook then boots a version nothing installed - which is worse than + # losing a reboot, and is the thing `arm_restart/4`'s order exists to prevent. + # The order is right; what was missing is that only one caller may be in it. + # + # The region has to reach further than the arming, for a second reason. + # `restart_planned?/3` is a prediction about the release the system is running, + # and an install that completes between it and `install_release/1` changes what + # `do_get_rh_script/4` will select: a concurrent hot upgrade moves the + # from-version, so a marker gets armed for a reboot OTP does not make, or a + # reboot happens with none armed. So the read, the classification, the arming, + # `install_release/1` and the disarming are all in here, and the ERTS guard - + # which reads two directories and can refuse without touching anything - is the + # only part deliberately left outside. + # + # `:global.trans/3` and no process of Castle's own, which is the point of + # choosing it: + # + # * `global_name_server` is a kernel process and is running whether or not + # distribution is, and `set_lock/2` over `[node()]` talks to the local one + # only. So this works on a node with `is_alive() == false`, which is the + # ordinary case for a release that configures no distribution and the case + # it was measured on. Nothing here needs a node name, epmd or a cookie. + # * `trans/3` releases the lock in an `after`, and `global` monitors the + # holder besides, so a caller that dies - an rpc whose far end went away - + # releases it instead of wedging every later install. + # * the alternative was a supervised process of Castle's own, and it is a + # worse trade. The modules here are deliberately stateless and every + # function runs inline in whatever process asked; a lock server would be a + # new thing in the *managed* system's supervision tree, with a lifetime and + # a restart strategy of its own, to serialise a command that runs a handful + # of times in a deployment's life. + # + # `[node()]` rather than the default `[node() | nodes()]` is deliberate. Every + # caller arrives here in the running node - `bin/castle` reaches it by `rpc`, + # and the launcher's preboot step only calls `make_releases/0` - so this node is + # the whole set of callers. A cluster-wide lock would make an install wait on + # nodes that share nothing with this deployment and make a partition its + # business, and it still would not cover a caller in some other VM. That is the + # boundary, said plainly: a second VM writing into this releases directory is + # outside the lock, and what is left there is the filesystem half - `publish/2` + # refusing rather than replacing - which is no worse than it was. + # + # It waits rather than refusing. An install that waited is a slow install; one + # refused because another was in flight is a failed one. The waiter goes on to + # find the winner's marker and be told that a restart install is pending, which + # is the message it would have got anyway - only now it is said about evidence + # that is complete, rather than said while destroying it. + # + # Retries are `infinity`, which `trans/3` is, so `set_lock/3` cannot answer + # `false` and there is no `aborted` for this to have to mean something by. + defp serialised(rel_dir, install) do + :global.trans({{@install_lock, rel_dir}, self()}, install, [node()]) + end + + # The install itself, with the running release asked for once, and the whole of + # what `serialised/2` holds the region open for. # # The record check and the restart prediction are both about the release the # system is running - `get_latest_release/1` is `current` if there is one and # `permanent` otherwise, which is what `running_release/1` computes - and asking - # `which_releases/0` twice would be asking about two moments. + # `which_releases/0` twice would be asking about two moments. Asking it once is + # not enough on its own: another caller's install can move the answer between + # the one question and the install that acts on it, which is the other half of + # why this whole function is inside the region rather than just the arming. defp install_upgradable(vsn, rel_dir, handler) do refusal = "Cannot install #{vsn}" running = running_release(handler) @@ -699,13 +791,22 @@ defmodule Castle.Commands do # 1. **Refuse if a marker is already there.** One pending restart install at # a time. Two attempts sharing one name overwrite and disarm each other, # and the survivor's marker says nothing about which of them - if either - - # reached `install_release/1`. `publish/2` is what actually decides this, - # by refusing rather than replacing; the look first is what keeps step 2 - # from destroying a pending attempt's evidence, since anything in flight - # holds the marker from before its own step 2 until its `disarm/2`. A - # marker that appears between the look and the publish belongs to an - # attempt that has not reached `install_release/1` yet, so there is - # nothing of its to destroy. + # reached `install_release/1`. So a marker at the path is a *finished* + # attempt's, waiting for the reboot it asked for, and this refusal is what + # keeps step 2 from clearing the `new_start_erl.data` that attempt's + # preparation wrote. `publish/2` decides it a second time over, by + # refusing rather than replacing. + # + # **What makes it true that the marker belongs to a finished attempt is + # `serialised/2` and not this look.** An earlier version of this argued + # that a marker appearing between the `lstat` and the publish belonged to + # an attempt that had not reached `install_release/1` yet, so there was + # nothing of its to destroy. That was the hole: two callers reach this + # check before either publishes, so both pass it, and the one that gets + # here second clears the first's file out from under a reboot that is + # already on its way. Do not reason about this step in isolation again - + # it is one caller's half of a rule whose other half is that there is only + # ever one. # 2. **Clear OTP's file.** `prepare_restart_new_emulator/7` writes # `releases/new_start_erl.data` and nothing removes it, so one left by an # earlier failure would pair with the marker armed next and boot a version @@ -725,8 +826,13 @@ defmodule Castle.Commands do # every later attempt. # # Steps 1 and 2 are in that order and must stay in it. Reversed, an attempt - # would refuse *after* clearing OTP's file, which is how a concurrent install - # loses its reboot silently. + # would refuse *after* clearing OTP's file, which is how an install that has + # already been asked for loses its reboot silently. + # + # This runs with no other caller in the install at all - `serialised/2` - and + # that is what the step 1 note above rests on. Neither replaces the other: the + # serialisation is why the marker means a finished attempt, and the order of + # these steps is why a refusal for any *other* reason still changes nothing. # # A failure at any step refuses the install rather than going ahead: the reboot # would come back on whichever version `releases/start_erl.data` names, which @@ -837,7 +943,10 @@ defmodule Castle.Commands do # between them is still removed. That window is two adjacent statements wide # and there is no POSIX operation that closes it - unlinking is by name, and no # name carries its identity. What it costs if it is ever hit is a lost reboot, - # which is the direction the whole protocol fails in. + # which is the direction the whole protocol fails in. It also takes something + # outside this node to hit at all now: this runs inside `serialised/2`, so the + # re-arming cannot be another install here, and what is left is a start of the + # deployment consuming the marker and some other VM arming one. # # The result of the removal is deliberately not looked at: a releases directory # this cannot remove the marker from is one `publish_marker/4` could not have diff --git a/test/castle/commands_test.exs b/test/castle/commands_test.exs index 7de8c0d..0cb9f1f 100644 --- a/test/castle/commands_test.exs +++ b/test/castle/commands_test.exs @@ -498,6 +498,118 @@ defmodule Castle.CommandsTest do end end + # The three steps of `arm_restart/4` are one caller's sequence, and two callers + # can run it at once: `release_handler` serialises `install_release/1`, but that + # is downstream of the read, the classification and the arming, so both callers + # get past the marker check before either publishes. The loser then clears the + # winner's `new_start_erl.data` after the winner's preparation wrote it, the + # winner's reboot comes back on the permanent release, and the loser says + # nothing has been changed while having changed it. + # + # So the whole install is serialised on the node, and these are about the region + # rather than about the marker: what they hold is that a second caller cannot + # get as far as the *lookup* while the first is inside. + describe "the restart marker, with two callers" do + @tag :tmp_dir + test "serialises them, so both cannot pass the initial lookup", %{tmp_dir: dir} do + # The barrier is `which_releases/0`, which is the first thing inside the + # region and the only seam in front of the arming - so a caller held there + # has taken the region and armed nothing, which is exactly the state two of + # them used to be able to occupy at once. Messages rather than sleeps: what + # is asserted is an order, and a test that waits for one is a test that + # sometimes asserts nothing. + relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) + + first = installer(dir, "1.2.2", as: :first, hold: true, install: prepares_then_reboots(dir)) + assert_receive {:looked_up, :first} + + second = installer(dir, "1.2.2", as: :second) + assert_receive {:started, :second} + + # The discriminator, and the whole of what the region buys. Without it the + # second caller reads the running release, classifies the same transition + # and passes `unclaimed/2` here, while the first is still in front of its + # own arming - after which one of them destroys the other's evidence. + refute_receive {:looked_up, :second}, 200 + + send(first.pid, :proceed) + assert {{:ok, lines}, [[~c"1.2.3"]]} = Task.await(first, 30_000) + assert Enum.join(lines, " ") =~ "The emulator is restarting." + + # Only now does the second caller look, and what it finds is a marker beside + # the file OTP's preparation wrote - a finished attempt waiting for its + # reboot. It is refused, which is the message it would have been given + # before as well; the difference is that it is now said about a pair that is + # complete rather than said while taking half of it away. + assert_receive {:looked_up, :second}, 10_000 + assert {{:error, message}, []} = Task.await(second, 30_000) + assert message =~ "a restart install is pending - it names 1.2.3" + + assert armed_version(dir) == "1.2.3" + + assert File.exists?(provisional(dir)), + "the waiting caller cleared the reboot's own new_start_erl.data" + end + + @tag :tmp_dir + test "hands the region on when the first install fails", %{tmp_dir: dir} do + # The other direction, and what says the region is given up on every way + # out rather than only on the happy one: a failed install disarms, so the + # caller that was waiting finds the path free and arms its own marker. + relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) + + failing = {:error, {:bad_relup_file, ~c"relup"}} + first = installer(dir, "1.2.2", as: :first, hold: true, install: failing) + assert_receive {:looked_up, :first} + + second = installer(dir, "1.2.2", as: :second, install: prepares_then_reboots(dir)) + assert_receive {:started, :second} + refute_receive {:looked_up, :second}, 200 + + send(first.pid, :proceed) + assert {{:error, _}, [[~c"1.2.3"]]} = Task.await(first, 30_000) + assert {{:ok, _}, [[~c"1.2.3"]]} = Task.await(second, 30_000) + + assert armed_version(dir) == "1.2.3" + end + + @tag :tmp_dir + test "classifies each of them from the release it found running", %{tmp_dir: dir} do + # One relup, two answers. From 1.2.1 the transition to 1.2.3 restarts the + # emulator and from 1.2.2 it is hot, so what the classification decides + # depends on the release the caller found running - and an install that + # completed in between would move it, arming a marker for a reboot OTP will + # not make or taking a reboot with none armed. Which is the second reason + # the region reaches past the arming to `install_release/1`. + relup!( + dir, + "1.2.3", + [{~c"1.2.2", [], [{:apply, {:m, :f, []}}]}, {~c"1.2.1", [], [:restart_emulator]}], + [] + ) + + restarting = + installer(dir, "1.2.1", as: :restarting, hold: true, install: prepares_then_reboots(dir)) + + assert_receive {:looked_up, :restarting} + + hot = installer(dir, "1.2.2", as: :hot) + assert_receive {:started, :hot} + refute_receive {:looked_up, :hot}, 200 + + send(restarting.pid, :proceed) + assert {{:ok, lines}, _} = Task.await(restarting, 30_000) + assert Enum.join(lines, " ") =~ "The emulator is restarting." + armed = File.read!(marker(dir)) + + # The hot caller arms nothing of its own, so it neither adopts nor disarms + # the marker the restarting one left: the reboot that is still owed happens + # on the version that asked for it. + assert {{:ok, ["Now running 1.2.3 (previously 1.2.2)."]}, _} = Task.await(hot, 30_000) + assert File.read!(marker(dir)) == armed + end + end + describe "running/3" do test "confirms the version an install has made current" do handler = @@ -811,17 +923,59 @@ defmodule Castle.CommandsTest do # What a node reports once its boot script has run to the end. defp booted, do: InitStub.stub({:starting, :started}) + # What `which_releases/0` reports for a node running `vsn` on a record it read + # from a RELEASES file: it names applications, so the check `unpack/3` and + # `install/3` make passes. + defp running_record(vsn) do + [{~c"sample", to_charlist(vsn), [~c"kernel-10.5", ~c"stdlib-7.2"], :permanent}] + end + # A handler whose running release was read from a RELEASES file, so it names # applications and the check unpack/3 and install/3 make passes, with `fun` # answering `reply`. defp real_record(fun, reply) do - Stub.stub(:which_releases, [ - {~c"sample", ~c"1.2.2", [~c"kernel-10.5", ~c"stdlib-7.2"], :permanent} - ]) - + Stub.stub(:which_releases, running_record("1.2.2")) Stub.stub(fun, reply) end + # An `install/4` of 1.2.3 in a task of its own, from a node running `from`, + # reporting where it got to over messages so that two of them can be + # interleaved deterministically. It answers `{result, install_release calls}`, + # because both are per-process: `Castle.ReleaseHandlerStub` keeps its replies + # and its record of calls in the dictionary of whichever process called it, and + # here that is the task rather than the test. + # + # `hold: true` stops the caller inside `which_releases/0` until it is sent + # `:proceed`. That is the seam the interleaving needs: the first thing the + # serialised region does and the last one before the marker is armed. + 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"}) + + Task.async(fn -> + Stub.stub(:which_releases, lookup) + Stub.stub(:install_release, reply) + + send(test, {:started, name}) + + {Commands.install("1.2.3", rel_dir, Stub), Stub.calls(:install_release)} + end) + 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 + fn _args -> + send(test, {:looked_up, name}) + if hold?, do: await_proceed() + running_record(from) + end + end + + defp await_proceed, do: receive(do: (:proceed -> :ok)) + # The same, for a node whose record release_handler synthesised because it # could not read RELEASES: the application list is empty. `fun` is registered # with the reply it would have given, so that asserting it was never called @@ -868,6 +1022,18 @@ defmodule Castle.CommandsTest do end end + # The same preparation, succeeding: `new_start_erl.data` written and the reply a + # one-stage restart is given, which is the one a completed hot upgrade is given + # too. That is the state a second caller must not be able to take apart - the + # marker and OTP's file, both this attempt's, waiting for a reboot that has been + # asked for and has not happened yet. + defp prepares_then_reboots(rel_dir) do + fn [vsn] -> + File.write!(provisional(rel_dir), "16.0 #{vsn}") + {:ok, ~c"1.2.2", ~c"upgrade"} + end + end + # A relup where `release_handler` reads one: `releases//relup`, holding a # single `{Vsn, Ups, Downs}` term. Written as a term rather than as text so that # what the classification consults is what `:file.consult/1` gives it, which is From f63f8df5b7dc52cf5bb75d6615c45f94f2d533cd Mon Sep 17 00:00:00 2001 From: ausimian Date: Sun, 23 Aug 2026 17:50:53 +1000 Subject: [PATCH 4/6] 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 --- AGENTS.md | 246 ++++++++++++++---- RELEASE.md | 49 +++- lib/castle.ex | 47 ++-- lib/castle/commands.ex | 416 +++++++++++++++++++++++------ lib/castle/deployment.ex | 58 ++++- test/castle/commands_test.exs | 448 ++++++++++++++++++++++++++++---- test/castle/erts_guard_test.exs | 10 +- test/support/deployment_stub.ex | 37 ++- test/support/peer_stub.ex | 8 + 9 files changed, 1109 insertions(+), 210 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4aca07d..dfe5c96 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -519,16 +519,27 @@ Castle's job is configuration and release management on a running node. API surface is [#11](https://github.com/ausimian/castle/issues/11)'s to settle. - **`unpack/1`, `install/1`, `commit/1`, `remove/1`, `releases/0`** — wrappers over `:release_handler`, with the target version's configuration materialised - ahead of `install` and `commit` so that it exists before the version is booted, - the record check inside `unpack` and `install`, and the ERTS guard inside all - of them but `releases/0`. The boundary composes - materialise-then-install, so a node that will be refused for its record - materialises the target's configuration before it hears so. That is what the - 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. Both - refusals fall before `install_release/1` is asked for anything, which is the - line that matters. + before `install` and `commit` hand it over, the record check inside `unpack` + and `install`, and the ERTS guard inside all of them but `releases/0`. + + **`Castle.install/1` composes nothing, and it used to.** It called + `Commands.materialise/3` and then `Commands.install/4`, and that composition + was the bug: two callers both configured the target before either reached the + lock. Materialising is now the third step *inside* `Commands.install/5`, after + the record check and after the pending-marker refusal and before the marker is + armed — so a node that will be refused, for its record or for a pending restart + install, is refused without having configured anything. The note this replaces + called materialising "only work" on the grounds that it writes into the target's + 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. + + `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. + + Every refusal still falls before `install_release/1` is asked for anything, + which is the line that matters. - **`Castle.running/1`** — succeeds when the version it is given is the release the system is running. `install_release/1`'s reply says only that the upgrade was accepted: a transition that restarts the emulator is replied to and then @@ -575,7 +586,7 @@ Castle's job is configuration and release management on a running node. emulator upgrade and says nothing about a script an operator supplies.) - **The restart marker** — `releases/castle-restart-pending`, armed by - `install/4` before `install_release/1` is asked for anything and cleared on + `install/5` before `install_release/1` is asked for anything and cleared on every path where the install failed. Forecastle's `env.sh` fragment consumes it on the next start and boots the version it names. The two halves are useless apart and landed together @@ -603,8 +614,8 @@ Castle's job is configuration and release management on a running node. disarming each other's marker. **So the pair is owned by an install *attempt*, and four things make it so.** - `arm_restart/4` is three of them and the order is the protocol; the fourth is - that there is only ever one caller in the install at all. + `unclaimed/3` and `arm/4` are three of them and the order is the protocol; the + fourth is that there is only ever one caller in the install at all. 1. **One pending restart install at a time.** A marker already at the path refuses the install rather than being adopted or replaced, which is what @@ -619,7 +630,7 @@ Castle's job is configuration and release management on a running node. attempt that refused *after* clearing would take an already-requested reboot away silently. 3. **The marker names the attempt that armed it**, on a second line, and - `disarm/2` removes it only if it still does. The marker's name is shared and + `disarm/3` removes it only if it still does. The marker's name is shared and the marker is short-lived — *any* `start` or `daemon` of the deployment consumes it, whether or not that start goes on to boot — so removing it by name would take a later attempt's marker away. The attempt is the operating @@ -691,12 +702,49 @@ Castle's job is configuration and release management on a running node. 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. The - materialisation `Castle.install/1` does *first* is outside it too, and - deliberately: it writes into the target's own version directory and touches no - release record, its own primitives already refuse rather than replace, and - holding this lock across a peer VM's boot would put every install behind - another's configuration step. Nothing about the marker protocol depends on it. + that is waiting on a reboot would be a deadlock dressed as caution. + + **Materialising the target's configuration is inside the region, and the + argument for keeping it outside was wrong.** That argument was: it writes only + into the target's own version directory, its own primitives refuse rather than + replace, and holding this lock across a peer VM's boot would put every install + behind another's configuration step. The middle claim is false about the step + that matters. The staging refuses rather than replaces and + `sys.config.pristine` refuses rather than replaces, but the *last* thing + materialising does is rename the resolved configuration onto `sys.config` — a + replace by design, and necessarily so, because that is the file + `release_handler` reads. So two callers materialising before either reached the + lock meant the loser's providers could replace the configuration the winner's + provisional release was about to boot, and the loser was then refused for the + winner's marker: a refused install decided what a successful one booted. That + providers may answer differently across evaluations is not a hypothetical — it + is the entire reason `sys.config.pristine` exists. + + The third claim is true and is not a reason. It is a throughput argument about + concurrent installs, and this protocol refuses concurrent installs anyway; an + install that waits is slow, and an install whose configuration is somebody + else's is wrong. + + **Inside the region is not enough on its own — it has to be after the + refusals.** `Commands.install_upgradable/5` runs the record check, then + `unclaimed/3`, then materialises, then arms. A caller refused for a pending + restart install must be refused *before* it configures anything, because the + version it would be configuring is the one the pending install's reboot is + about to boot. Moving the materialisation inside the lock while leaving it in + 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. **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 @@ -759,11 +807,63 @@ Castle's job is configuration and release management on a running node. `install_release/1` is asked for anything, and so does a stale `new_start_erl.data` that cannot be cleared — because the alternative in both cases is a reboot that comes back on the wrong version with nothing saying so. - Clearing the marker, by contrast, is best-effort on purpose: a releases - directory the marker cannot be removed from is one it could not have been - linked into, and that already refused. - **The report changes with it.** `installed/4` says the version was installed, + **Settling the marker afterwards happens on every way out, including the ones + that do not return, and failing to settle it is reported.** Both halves of that + replaced something weaker. + + The region is `Commands.installed/5`, and it is an *implicit* `try` — the + function body, with `catch` and `else` clauses, which is the form + `credo --strict` asks for. It used to be a bare `case` over the reply with + `disarm` in the two failing branches, so an exit, a throw or a raise out of + `install_release/1` went past both: the marker stayed armed, and where + `prepare_restart_new_emulator/7` had already written `new_start_erl.data` the + pair was complete and the next start booted a version whose install had blown + up. That is the hazard the whole protocol exists to prevent, reintroduced + through the one path that is not a return. It is `catch`/`else` and **not** + `after`: an `after` cannot see which way the block went, so it would disarm the + successful restart install too, taking away the marker whose entire purpose is + to outlive the call. There is a test for that. + + An exception is re-raised unchanged once the marker is settled — `Castle` is + the boundary that raises, `Kernel.CLI` catches on the node and the calling VM + re-raises, and Castle has nothing to add to an exception out of + `release_handler` that is worth losing the stacktrace for. The one exception to + that is a marker it could not settle, where the exception is folded into the + message instead, because that is the fact an operator most needs and a + stacktrace is where it would be buried. + + Clearing the marker used to be best-effort on the argument that a releases + directory the marker cannot be removed from is one it could not have been + linked into, so the install would already have refused. **That holds only if + nothing changed in between, and `install_release/1` runs in between** — for as + long as an upgrade takes, with the system's own code being replaced. Worse, an + *unreadable* marker was classified as another attempt's and left alone, which + reads as caution and is not: a marker that cannot be read is no evidence about + whose it is, and the file it might be is the one the next start acts on. + + So `disarm/3` has four answers and two of them are failures. **Ours** is + removed, and a removal that fails is reported. **Theirs** is left, which is a + success — a later attempt's marker is a reboot still owed. **Gone** is a start + of the deployment having consumed it, which is the outcome that was wanted, so + `:enoent` from either the read or the removal is success. **Unverifiable** is + reported: Castle will not remove a marker it cannot show is its own, and will + not pretend the question was answered. What the operator is told names the + file, says `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 before the system is restarted. + + The read and the removal go through `Castle.Deployment.read/1` and `rm/1`, for + the reason `stat/1` is there: the answers that decide what Castle *says* are + the failing ones, and every fixture that makes a `read` or `rm` fail on a + regular file in a writable directory does it with a mode, which root and some + filesystems ignore — so the fixture would only sometimes describe the state it + names and would pass either way. That seam is for outcomes Castle has to speak + about and cannot cause; it is **not** a general filesystem seam, and the + primitives that *publish* the marker stay called directly for the reason given + above. + + **The report changes with it.** `reported/5` says the version was installed, that the emulator is restarting, and that the version stays provisional until it is committed — instead of "Now running", which is false for as long as the reboot takes and which automation reads. @@ -788,7 +888,7 @@ module. | --- | --- | | `lib/castle.ex` | The command boundary: print the outcome, or raise | | `lib/castle/commands.ex` | The commands themselves, returning their outcome | -| `lib/castle/deployment.ex` | The two environment facts the ERTS guard rests on, and nothing else | +| `lib/castle/deployment.ex` | The facts about the deployment Castle cannot arrange and a test cannot produce: the two roots, and the `stat`/`read`/`rm` whose *failures* decide what a refusal says | | `lib/castle/peer.ex` | The temporary VM that runs the target's own config providers, both sides of it | | `lib/castle/error.ex` | The exception a failed command raises | | `test/support/` | Stubs for `:release_handler`, `:init`, the peer, the deployment and config providers, plus the release-shaped tree a real peer is booted on | @@ -1007,7 +1107,7 @@ unobservable things assertable: a preparation that writes `new_start_erl.data` and *then* fails, so that a same-version retry can be shown to clear it rather than pair with it; the filesystem as a hard restart before the reboot would find it, which is the marker alone and no pair; and a marker replaced between the -arming and the disarming, so that `disarm/2` can be shown to leave a marker it +arming and the disarming, so that `disarm/3` can be shown to leave a marker it did not write. None of those has an end state that distinguishes it — a successful install leaves the marker armed either way, and a failed one leaves it gone either way — which is the same reason `Castle.Peer`'s primitives are public. @@ -1021,26 +1121,70 @@ attempt's evidence. `install_release/1`.** The state that used to be reachable is two callers past the running-release lookup and neither of them armed, so what has to be held open is the *front* of the serialised region — and the lookup is both the first thing in -it and the last thing before the marker is armed. So `installer/3` runs -`install/4` in a task of its own with a `which_releases/0` that reports where it +it and the last thing before anything is written. So `installer/3` runs +`install/5` in a task of its own with a `which_releases/0` that reports where it got to and, optionally, waits to be released; a second caller is started while the first is held there, and the discriminator is that its lookup never happens -(`refute_receive {:looked_up, :second}`). That is the only kind of assertion -available, and it is honest about why: the end state is the *same* either way — -the second caller is refused for a marker it finds and changes nothing — because -the interleaving that destroys evidence needs a caller suspended between step 1 -and step 2, which has no seam and needs none once there cannot be two. -`Task.await` and `assert_receive` carry generous timeouts because `global`'s lock -retry backs off by up to a second or two, and every stub reply and call record -lives in the *task's* process dictionary, so a caller answers with its own -`Stub.calls(:install_release)` rather than the test reading them. - -Three of them, and the third is not about the marker: one relup for 1.2.3 whose -transition from 1.2.2 is hot and from 1.2.1 restarts the emulator, installed -concurrently by two callers running different versions. It says the classification -belongs to the caller that made it — which is the second half of why the region -reaches past the arming — and that the hot caller neither adopts nor disarms the -marker the restarting one is waiting on. +(`refute_receive {:looked_up, :second}`). `Task.await` and `assert_receive` carry +generous timeouts because `global`'s lock retry backs off by up to a second or +two, and every stub reply and call record lives in the *task's* process +dictionary, so a caller answers with its own `Stub.calls(:install_release)` and +its own `PeerStub.calls()` rather than the test reading them. + +Four of them. The second is the other direction — a failed first install hands the +region on, and the waiter arms its own marker. The third is not about the marker +at all: one relup for 1.2.3 whose transition from 1.2.2 is hot and from 1.2.1 +restarts the emulator, installed concurrently by two callers running different +versions. It says the classification belongs to the caller that made it — the +second half of why the region reaches past the arming — and that the hot caller +neither adopts nor disarms the marker the restarting one is waiting on. + +**The fourth is about the configuration, and it is the one that needs providers +whose results can be told apart.** `Castle.PeerStub` therefore accepts a +*function* reply, like `Castle.ReleaseHandlerStub` does, so a caller's +materialisation can write a distinguishable `sys.config` into the version +directory — which is what the real one ends by renaming into place. Two callers +are given different ones, the first is held at the lookup and goes on to arm and +"reboot", the second is refused for the pending marker, and the assertions are +that the second's peer was **never called** (`PeerStub.calls() == []`) and that +the target is left holding the first's configuration. With both callers answering +`{:ok, []}` there is nothing to see: the end state is identical whichever of them +ran, which is exactly why the composition survived three rounds of review. Note +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. + +**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 +`new_start_erl.data` written first in the raise case so that what survives would +be the complete pair. A fourth asserts the opposite for a *successful* restart +install — the marker stays — which is what forbids `try/after` in place of +`catch`/`else`, since an `after` cannot tell the two apart and no other assertion +here would notice. + +**Being unable to settle the marker is reached through `Castle.DeploymentStub`, +not through a mode.** `stub_read/1` and `stub_rm/1` take a reply or a function of +the path, so a refusal can be scoped to the marker alone while everything else the +install touches goes through the real `File`. Unstubbed, both are the real thing, +for the reason `stub_stat/1` is: a fixture that only sometimes turns on the state +it names is a test that only sometimes tests anything. The four cases are a +removal refused with `:eacces`, a read refused with `:eio`, both of those *and* a +raise from `install_release/1` — where the exception has to be folded into the +message rather than let out — and the quiet one, a marker a start of the +deployment consumed, where `:enoent` must not be reported as a failure. That last +one is what keeps the reporting from being noise on an ordinary interrupted +install. The `DeploymentStub` in these is given `nil` for both roots, so the ERTS +guard is inert exactly as it is under `mix test` with no `RELEASE_ROOT`. + +Every install case now names a `configured(dir)` — the version directory unpacked +and a peer that says it configured it — because materialising is a step of the +install rather than something composed in front of it. That is deliberate rather +than an inconvenience: a case that did not say what the peer did would not have +said what the version it installed is configured with. The ERTS-guard cases pass +an **unstubbed** `Castle.PeerStub` instead, which raises if it is reached, so +"refuses without starting a peer" is asserted by the guard holding rather than by +a separate look. What is *not* covered here is a booted release: the upgrade of a running system, and the exit statuses `bin/castle` returns, belong to Forecastle's @@ -1107,11 +1251,23 @@ wrote, so Elixir's pipeline is still armed in the file the launcher reads. `Castle.install/1` in a VM of its own against the same deployment gets the filesystem half of the protocol and nothing more: `publish/2` refuses rather than replaces, so the marker cannot be silently taken over, and the window the - lock closes — two callers both past `unclaimed/2` before either publishes — is + lock closes — two callers both past `unclaimed/3` before either publishes — is 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. - **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)). diff --git a/RELEASE.md b/RELEASE.md index 8db957d..f40e5ec 100644 --- a/RELEASE.md +++ b/RELEASE.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 diff --git a/lib/castle.ex b/lib/castle.ex index 7c63cff..e9f40ff 100644 --- a/lib/castle.ex +++ b/lib/castle.ex @@ -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 diff --git a/lib/castle/commands.ex b/lib/castle/commands.ex index 4609350..d4ef6c1 100644 --- a/lib/castle/commands.ex +++ b/lib/castle/commands.ex @@ -14,7 +14,7 @@ defmodule Castle.Commands do # differ; it has said nothing, which is a different answer. @no_inode "the filesystem reports no inode numbers, so the two cannot be compared" - # The file `install/4` arms before a transition that reboots the emulator, and + # The file `install/5` arms before a transition that reboots the emulator, and # the launcher's `env.sh` fragment consumes on the next start. It sits beside # the release records, and it is named to be unmistakable: nothing else writes # it, and a human who finds one knows what it is for. @@ -27,8 +27,8 @@ defmodule Castle.Commands do # together are the evidence: OTP's file says which version, and this one says # that Castle asked for the reboot that would boot it. # - # **Agreeing on a version is not enough, and believing it was is what - # `arm_restart/4` had to be rewritten for.** Two files that merely name the + # **Agreeing on a version is not enough, and believing it was is what the + # arming protocol had to be rewritten for.** Two files that merely name the # same version say nothing about being the work of one install: a failed # attempt to X leaves OTP's file naming X, a retry to X arms a fresh marker # beside it, and a hard restart before the retry reaches `install_release/1` @@ -36,7 +36,7 @@ defmodule Castle.Commands do # boots X with OTP's records calling it `unpacked`. So the pair has to belong # to one *attempt*, which is `@provisional_marker` being cleared before the # marker is armed, the marker being published exclusively, and the marker - # naming the attempt that wrote it. See `arm_restart/4`. + # naming the attempt that wrote it. See `unclaimed/3` and `arm/4`. # # Those three are about one caller's sequence, and they are not on their own # enough either: two callers can run the sequence at once, and then the loser @@ -355,7 +355,7 @@ defmodule Castle.Commands do This is the question on its own, for an operator who wants it answered without acting on the answer. It is not what protects an upgrade: `unpack/3` and - `install/3` ask it themselves, from inside the operation, because an answer + `install/5` ask it themselves, from inside the operation, because an answer given to one caller and acted on by another is an answer about a moment that has passed - the node can restart in between, and the node that comes back synthesises the record afresh. Nothing has to call this first, and putting it @@ -376,7 +376,7 @@ defmodule Castle.Commands do end end - # The check `unpack/3` and `install/3` make before they touch + # The check `unpack/3` and `install/5` make before they touch # `:release_handler`, and what `upgradable/1` answers on its own. # # `refusal` is what the message leads with, and it names the operation rather @@ -388,7 +388,7 @@ defmodule Castle.Commands do end # The same rule over a running release that has already been asked for. - # `install/4` needs it twice - the record check, and which relup entry the + # `install/5` needs it twice - the record check, and which relup entry the # transition will be evaluated from - and `which_releases/0` must be asked once: # two calls are two moments, which is the whole point of the check being inside # the operation. @@ -445,11 +445,17 @@ defmodule Castle.Commands do test can see what was asked of it without starting a VM. Gated on the release bringing its own ERTS - see `ensure_own_erts/2` - because - `Castle.install/1` and `Castle.commit/1` do this first, and `rel_vsn_dir` is - derived from `code:root_dir()`: without the guard the operator's first news of - an ERTS-less deployment is that some version directory inside the Erlang - installation holds no release to configure, which is true and says nothing - about why. + `rel_vsn_dir` is derived from `code:root_dir()`, so without the guard the + operator's first news of an ERTS-less deployment is that some version directory + inside the Erlang installation holds no release to configure, which is true and + says nothing about why. + + The guard is redundant for `install/5`, which makes it before it takes the lock + and so before it reaches here, and it is not redundant for `Castle.commit/1`, + which still composes this in front of `commit/3` the way `Castle.install/1` + used to. It stays either way: this is a public entry point of its own, and a + guard that is only correct because of who happens to call it is one call away + from being wrong. """ @spec materialise(Path.t(), module(), module()) :: result() def materialise(rel_vsn_dir, peer \\ Peer, deployment \\ Castle.Deployment) do @@ -543,18 +549,52 @@ defmodule Castle.Commands do it from `code:root_dir()`, and a test needs somewhere to look. A transition that reboots the emulator is refused, with nothing touched, while - another such install is still pending - see `arm_restart/4`. One at a time is - the price of the marker being evidence about a particular install rather than - about a version. + another such install is still pending - see `unclaimed/3`. One at a time is the + price of the marker being evidence about a particular install rather than about + a version. Two installs cannot be under way on this node at once at all - see `serialised/2`. The refusal above is what the second one is then told, once the first has finished and its marker is complete. + + **Materialising the target's configuration is part of this operation, and used + to be composed in front of it.** `Castle.install/1` called + `materialise/3` and then this, so two callers both materialised before either + reached the lock. That is not the harmless idempotent work it was argued to be: + materialisation *ends in a rename onto the target's `sys.config`*, which is a + replace by design, because that is the file `:release_handler` reads and so the + file the resolved configuration has to land in. The staging refuses rather than + replaces and `sys.config.pristine` refuses rather than replaces; the last step + does neither, and cannot. So the loser's providers - evaluated in a VM of their + own, over whatever environment that caller had - overwrote the configuration + the winner's provisional release was about to boot, and the loser was then + refused for the winner's marker. A refused install decided what a successful + one booted. + + It is inside the region and *after* the refusals, which is the half that + matters and the half a "move it inside the lock" would have missed: a caller + that is going to be told a restart install is pending must not materialise on + its way to being told. See `install_upgradable/5` for the order and why each + step is where it is. + + The peer comes before the deployment, the way `materialise/3` takes them, and + the order earns something in the tests as well as being consistent: the cases + about the ERTS guard hand this an unstubbed `Castle.PeerStub`, which raises if + it is reached, so "refuses without starting a peer" is asserted by the guard + holding rather than by a separate look. """ - @spec install(String.t(), Path.t(), module(), module()) :: result() - def install(vsn, rel_dir, handler \\ :release_handler, deployment \\ Castle.Deployment) do + @spec install(String.t(), Path.t(), module(), module(), module()) :: result() + def install( + vsn, + rel_dir, + handler \\ :release_handler, + peer \\ Peer, + deployment \\ Castle.Deployment + ) do with :ok <- ensure_own_erts("Cannot install #{vsn}", deployment) do - serialised(rel_dir, fn -> install_upgradable(vsn, rel_dir, handler) end) + serialised(rel_dir, fn -> + install_upgradable(vsn, rel_dir, handler, peer, deployment) + end) end end @@ -568,11 +608,11 @@ defmodule Castle.Commands do # Everything after the ERTS guard, run with no other caller in it. # - # **The three steps of `arm_restart/4` are correct for one caller and say + # **The steps of the arming protocol are correct for one caller and say # nothing across processes, and `release_handler` serialising `install_release/1` # does not close that.** Its serialisation is *downstream* of the whole # protocol: two callers can both read the running release, both classify it, - # and both pass `unclaimed/2`, because all of that happens before either of + # and both pass `unclaimed/3`, because all of that happens before either of # them publishes anything. Refusing before clearing then buys nothing. The # loser's `clear_provisional/3` runs after the winner's `install_release/1` # has written `new_start_erl.data`, so it deletes the winner's live evidence; @@ -583,7 +623,7 @@ defmodule Castle.Commands do # **Do not reorder the protocol to avoid that.** Publishing before clearing # leaves a window in which the marker pairs with a *stale* `new_start_erl.data`, # and the hook then boots a version nothing installed - which is worse than - # losing a reboot, and is the thing `arm_restart/4`'s order exists to prevent. + # losing a reboot, and is the thing the protocol's order exists to prevent. # The order is right; what was missing is that only one caller may be in it. # # The region has to reach further than the arming, for a second reason. @@ -592,9 +632,27 @@ defmodule Castle.Commands do # `do_get_rh_script/4` will select: a concurrent hot upgrade moves the # from-version, so a marker gets armed for a reboot OTP does not make, or a # reboot happens with none armed. So the read, the classification, the arming, - # `install_release/1` and the disarming are all in here, and the ERTS guard - - # which reads two directories and can refuse without touching anything - is the - # only part deliberately left outside. + # `install_release/1` and the disarming are all in here. + # + # **And so is materialising the target's configuration, which was the last thing + # left outside and did not belong there.** The argument for keeping it out was + # that it writes only into the target's own version directory, that its + # primitives refuse rather than replace, and that holding this lock across a + # peer VM's boot would put every install behind another's configuration step. + # The first two are wrong about the step that matters - the *rename onto + # `sys.config`* replaces, by design and necessarily - and the third is a + # throughput argument about concurrent installs, which this protocol refuses + # anyway. An install that waits is slower; an install whose configuration is + # somebody else's is wrong. + # + # The ERTS guard is now the only part deliberately left outside: it reads two + # directories and can refuse without touching anything, and a refusal has no + # reason to queue behind a reboot. + # + # What is still outside this and still writes a `sys.config` is `commit/3`'s + # materialisation, which `Castle.commit/1` composes the way `install/1` used to. + # It is not the same case - see `Castle.materialise/1` - but it is a boundary + # rather than a proof, and it is written down as one in AGENTS.md. # # `:global.trans/3` and no process of Castle's own, which is the point of # choosing it: @@ -646,24 +704,89 @@ defmodule Castle.Commands do # not enough on its own: another caller's install can move the answer between # the one question and the install that acts on it, which is the other half of # why this whole function is inside the region rather than just the arming. - defp install_upgradable(vsn, rel_dir, handler) do + # + # **The order of the four steps is the protocol, and materialising is the third + # of them.** Read as a rule: nothing that writes runs until everything that can + # refuse has been asked. + # + # 1. `refuse_synthesised/2` - a fact about this node's release record. + # 2. `unclaimed/3` - a restart install is already pending. This is step 1 of + # what used to be `arm_restart/4`, pulled in front of the materialisation + # *because* of it: a caller that is going to be refused here must not have + # replaced the target's `sys.config` on its way to being told, or the + # configuration the pending install's reboot boots is the refused caller's. + # 3. `materialise/3` - the target's own providers, in a VM of their own, + # resolved onto the target's `sys.config`. First thing here that writes + # anything, and the last thing that can refuse for a reason about the + # target rather than about this node. + # 4. `arm/4` - clear OTP's file, then publish the marker. The first + # *destructive* step, and it stays after everything above for the reason + # its own note gives: an attempt that refused after clearing would take an + # already-requested reboot away in silence. + # + # Materialising before the record check is what this used to do, and the note in + # `Castle` called it "only work". It is not: see `install/5`. Materialising + # after step 4 would be worse still - the marker would be armed for an install + # that a provider could then refuse, and `install_release/1` is the line nothing + # may fail after without saying an install happened. + defp install_upgradable(vsn, rel_dir, handler, peer, deployment) do refusal = "Cannot install #{vsn}" running = running_release(handler) restart? = restart_planned?(vsn, rel_dir, running) with :ok <- refuse_synthesised(refusal, running), - {:ok, attempt} <- arm_restart(restart?, vsn, rel_dir, refusal) do - installed(vsn, attempt, rel_dir, handler) + :ok <- unclaimed(restart?, rel_dir, refusal), + {:ok, configured} <- materialise(Path.join(rel_dir, vsn), peer, deployment), + {:ok, attempt} <- arm(restart?, vsn, rel_dir, refusal), + {:ok, lines} <- installed(vsn, attempt, rel_dir, handler, deployment) do + {:ok, configured ++ lines} end end + # The armed region: `install_release/1`, with the marker's ownership settled on + # **every** way out of it. + # + # It used to be a bare `case` over the reply, with `disarm/2` in the two failing + # branches - so an exit, a throw or a raise from `install_release/1` left the + # region without settling anything. That is the "boots a version nothing + # installed" hazard the whole marker protocol exists to prevent, reintroduced + # through the one path that does not return: if + # `prepare_restart_new_emulator/7` has already written `new_start_erl.data` by + # then, an exception leaves two agreeing files and the next start consumes them. + # + # `try/catch/else` rather than `after`, and the distinction is the point. An + # `after` cannot see which way the block went, so it would disarm on the + # *successful* restart install too - taking away the marker whose whole purpose + # is to outlive this call. The `else` clause is the returns, the `catch` clause + # is the ones that are not returns, and only the second class is a failure the + # caller has not been told about yet. + # + # An exception is re-raised rather than turned into a message, when the marker + # could be settled: `Castle` is the boundary that raises, `Kernel.CLI` catches + # on the node and the calling VM re-raises with the reason and a non-zero exit, + # and Castle has nothing to add to an exception out of `:release_handler` that + # is worth losing the stacktrace for. What it does have something to say about + # is a marker it could not settle, and that is the one case where this reports + # instead - see `abandoned/5`. + # The function body *is* the `try`, which is what `credo --strict` asks for and + # is why there is no visible `try do` here. The `catch` and `else` below belong + # to it, and the whole of `installed/5` is the guarded region. + defp installed(vsn, attempt, rel_dir, handler, deployment) do + handler.install_release(to_charlist(vsn)) + catch + kind, reason -> + abandoned(vsn, attempt, rel_dir, deployment, {kind, reason, __STACKTRACE__}) + else + outcome -> reported(vsn, attempt, rel_dir, deployment, outcome) + end + # `install_release/1` replies the same `{ok, Vsn, Descr}` for a hot upgrade and # for one that is about to reboot, so what it says cannot tell them apart - # which is why the transition is classified from the relup beforehand and the # answer carried in here. Reporting a reboot as "now running" is a claim that is # false for as long as the reboot takes, and automation reads it. - defp installed(vsn, attempt, rel_dir, handler) do - case handler.install_release(to_charlist(vsn)) do + defp reported(vsn, attempt, rel_dir, deployment, outcome) do + case outcome do {:ok, other_vsn, _descr} when is_binary(attempt) -> {:ok, [ @@ -688,15 +811,56 @@ defmodule Castle.Commands do ]} {:error, reason} -> - disarm(attempt, rel_dir) - {:error, "Install of #{vsn} failed. #{inspect(reason)}"} + failed(rel_dir, attempt, deployment, "Install of #{vsn} failed. #{inspect(reason)}") other -> - disarm(attempt, rel_dir) - {:error, "Install of #{vsn} returned an unexpected result. #{inspect(other)}"} + failed( + rel_dir, + attempt, + deployment, + "Install of #{vsn} returned an unexpected result. #{inspect(other)}" + ) + end + end + + # A failure `install_release/1` reported. The marker goes, and if it cannot the + # operator is told so *as well as* the failure, rather than instead of it: the + # install failing is what they asked about, and a stranded marker is a second + # fact about what the next start of the system will now do. + defp failed(rel_dir, attempt, deployment, message) do + case disarm(attempt, rel_dir, deployment) do + :ok -> {:error, message} + {:stranded, why} -> {:error, "#{message} #{stranded(rel_dir, why)}"} end end + # A failure `install_release/1` did not report, because it exited, threw or + # raised. The marker is settled first - that is the whole point of catching - + # and then the original failure is allowed out unchanged. + # + # Unless the marker could not be settled, in which case it is *not* allowed out + # unchanged, because the thing the operator most needs told would be the thing + # the stacktrace buries. The exception is folded into the message instead, + # formatted the way an unhandled one would have been printed, so nothing is + # lost - and the reason it can be folded in at all is that this is the one + # branch where Castle knows something the exception does not say. + defp abandoned(vsn, attempt, rel_dir, deployment, {kind, reason, stack}) do + case disarm(attempt, rel_dir, deployment) do + :ok -> + :erlang.raise(kind, reason, stack) + + {:stranded, why} -> + {:error, + "Install of #{vsn} #{describe_exit(kind)}, and the restart marker it " <> + "armed could not be settled. #{stranded(rel_dir, why)} The failure " <> + "itself: #{Exception.format(kind, reason, stack)}"} + end + end + + defp describe_exit(:error), do: "raised" + defp describe_exit(:throw), do: "threw" + defp describe_exit(:exit), do: "exited" + ## The restart marker # Whether the transition about to be installed reboots the emulator into a @@ -783,38 +947,47 @@ defmodule Castle.Commands do end) end - # Armed before `install_release/1` is asked for anything, so that the marker is - # in place before OTP writes its own and reboots, and armed as **this - # attempt's** rather than as the version's. Three steps, in this order, and the - # order is the protocol: + # Step 2 of `install_upgradable/5`: **refuse if a marker is already there.** + # + # One pending restart install at a time. Two attempts sharing one name overwrite + # and disarm each other, and the survivor's marker says nothing about which of + # them - if either - reached `install_release/1`. So a marker at the path is a + # *finished* attempt's, waiting for the reboot it asked for, and this refusal is + # what keeps `clear_provisional/3` from clearing the `new_start_erl.data` that + # attempt's preparation wrote. `publish/2` decides it a second time over, by + # refusing rather than replacing. # - # 1. **Refuse if a marker is already there.** One pending restart install at - # a time. Two attempts sharing one name overwrite and disarm each other, - # and the survivor's marker says nothing about which of them - if either - - # reached `install_release/1`. So a marker at the path is a *finished* - # attempt's, waiting for the reboot it asked for, and this refusal is what - # keeps step 2 from clearing the `new_start_erl.data` that attempt's - # preparation wrote. `publish/2` decides it a second time over, by - # refusing rather than replacing. + # **What makes it true that the marker belongs to a finished attempt is + # `serialised/2` and not this look.** An earlier version of this argued that a + # marker appearing between the `lstat` and the publish belonged to an attempt + # that had not reached `install_release/1` yet, so there was nothing of its to + # destroy. That was the hole: two callers reach this check before either + # publishes, so both pass it, and the one that gets here second clears the + # first's file out from under a reboot that is already on its way. Do not reason + # about this step in isolation again - it is one caller's half of a rule whose + # other half is that there is only ever one. # - # **What makes it true that the marker belongs to a finished attempt is - # `serialised/2` and not this look.** An earlier version of this argued - # that a marker appearing between the `lstat` and the publish belonged to - # an attempt that had not reached `install_release/1` yet, so there was - # nothing of its to destroy. That was the hole: two callers reach this - # check before either publishes, so both pass it, and the one that gets - # here second clears the first's file out from under a reboot that is - # already on its way. Do not reason about this step in isolation again - - # it is one caller's half of a rule whose other half is that there is only - # ever one. - # 2. **Clear OTP's file.** `prepare_restart_new_emulator/7` writes + # **It is in front of the materialisation rather than inside `arm/4`, and that + # is not tidying.** A caller refused here has not written anything, and it must + # not have: the pending install it is being told about is going to reboot into + # the version whose `sys.config` this caller would otherwise have replaced on + # its way to the refusal. Being refused and having decided the winner's + # configuration are what this ordering keeps apart. + defp unclaimed(false, _rel_dir, _refusal), do: :ok + defp unclaimed(true, rel_dir, refusal), do: unclaimed(rel_dir, refusal) + + # Steps 4a and 4b, run together because nothing may come between them: the + # marker has to be in place before OTP writes its own and reboots, and it is + # armed as **this attempt's** rather than as the version's. + # + # a. **Clear OTP's file.** `prepare_restart_new_emulator/7` writes # `releases/new_start_erl.data` and nothing removes it, so one left by an # earlier failure would pair with the marker armed next and boot a version # this attempt never installed. Removing it is safe: `write_new_start_erl/3` # goes through `file:write_file/2`, which creates the file when it is # absent. After this, that file existing means *this* attempt's # preparation wrote it. - # 3. **Publish the marker.** Staged in an owner-only working directory and + # b. **Publish the marker.** Staged in an owner-only working directory and # hard-linked into place, which is how `Castle.Peer` publishes # `sys.config.pristine` and for the same two reasons: a link publishes a # file that is already complete, so no start can read a marker that is @@ -825,32 +998,31 @@ defmodule Castle.Commands do # write, and a death in that window leaves an empty marker that blocks # every later attempt. # - # Steps 1 and 2 are in that order and must stay in it. Reversed, an attempt - # would refuse *after* clearing OTP's file, which is how an install that has - # already been asked for loses its reboot silently. + # The refusal in `unclaimed/3` comes before a) and must stay there. Reversed, an + # attempt would refuse *after* clearing OTP's file, which is how an install that + # has already been asked for loses its reboot silently. # # This runs with no other caller in the install at all - `serialised/2` - and - # that is what the step 1 note above rests on. Neither replaces the other: the + # that is what the `unclaimed/3` note rests on. Neither replaces the other: the # serialisation is why the marker means a finished attempt, and the order of # these steps is why a refusal for any *other* reason still changes nothing. # - # A failure at any step refuses the install rather than going ahead: the reboot - # would come back on whichever version `releases/start_erl.data` names, which - # is the one being upgraded away from, and the upgrade would be lost with + # A failure at either step refuses the install rather than going ahead: the + # reboot would come back on whichever version `releases/start_erl.data` names, + # which is the one being upgraded away from, and the upgrade would be lost with # nothing saying so. - defp arm_restart(false, _vsn, _rel_dir, _refusal), do: {:ok, nil} + defp arm(false, _vsn, _rel_dir, _refusal), do: {:ok, nil} - defp arm_restart(true, vsn, rel_dir, refusal) do + defp arm(true, vsn, rel_dir, refusal) do attempt = attempt() - with :ok <- unclaimed(rel_dir, refusal), - :ok <- clear_provisional(rel_dir, vsn, refusal), + with :ok <- clear_provisional(rel_dir, vsn, refusal), :ok <- publish_marker(rel_dir, vsn, attempt, refusal) do {:ok, attempt} end end - # What names this attempt, and the whole of what `disarm/2`'s ownership rests + # What names this attempt, and the whole of what `disarm/3`'s ownership rests # on. The operating system pid, the wall clock in nanoseconds and a number no # other call in this VM will use again: unique within a node, and unique across # a node's restarts, which is as far as ownership has to reach - the marker @@ -926,7 +1098,8 @@ defmodule Castle.Commands do defp armed(:taken, marker, refusal), do: {:error, pending(marker, refusal)} defp armed({:error, reason}, marker, refusal), do: {:error, unarmed(marker, reason, refusal)} - # Cleared on every path out of a failed install, because a marker left armed + # Cleared on every path out of a failed install - including the ones that do not + # return, which is what `installed/5` catches for - because a marker left armed # beside a `new_start_erl.data` the same failure may already have written is # exactly the pair the hook acts on. # @@ -948,29 +1121,106 @@ defmodule Castle.Commands do # re-arming cannot be another install here, and what is left is a start of the # deployment consuming the marker and some other VM arming one. # - # The result of the removal is deliberately not looked at: a releases directory - # this cannot remove the marker from is one `publish_marker/4` could not have - # linked it into, and that refuses the install before `install_release/1` is - # asked for anything. - defp disarm(nil, _rel_dir), do: :ok + # **Failing to settle it is reported, and used not to be.** The argument for + # ignoring `File.rm/1`'s result was that a releases directory the marker cannot + # be removed from is one `publish_marker/4` could not have linked it into, so + # the install would already have been refused. That holds only if nothing + # changed in between, and `install_release/1` runs in between - for as long as + # an upgrade takes, with the whole system's code being replaced. A mode applied + # underneath it, a mount that went read-only, or the marker being replaced by + # something that is not a file are all reachable from there. + # + # The unreadable case was worse than ignored, it was *misclassified*: an + # unreadable marker was treated as another attempt's and left alone. A marker + # that cannot be read is not evidence that it is somebody else's; it is no + # evidence at all, and the file it might be is the one the next start acts on. + # + # So there are four answers and only two of them are `:ok`: + # + # * **ours** - remove it, and report a removal that failed. + # * **theirs** - leave it, and that is a success: a later attempt's marker is + # a reboot that is still owed. + # * **gone** - a start of the deployment consumed it. Nothing to do. + # * **unverifiable** - say so. Castle will not remove a marker it cannot show + # is its own, and it will not pretend the question was answered. + # + # `:enoent` from the removal is `:ok` for the same reason **gone** is: the check + # and the removal are two calls, so a marker consumed between them is a marker + # that is no longer there, which is the outcome that was wanted. + defp disarm(nil, _rel_dir, _deployment), do: :ok - defp disarm(attempt, rel_dir) do + defp disarm(attempt, rel_dir, deployment) do marker = Path.join(rel_dir, @restart_marker) - if armed_by?(marker, attempt) do - File.rm(marker) + case armed_by(marker, attempt, deployment) do + :ours -> removed(marker, deployment) + :theirs -> :ok + :gone -> :ok + {:unverifiable, reason} -> {:stranded, unverifiable(marker, reason)} end + end - :ok + defp armed_by(marker, attempt, deployment) do + case deployment.read(marker) do + {:ok, contents} -> whose(contents, attempt) + {:error, :enoent} -> :gone + {:error, reason} -> {:unverifiable, reason} + end end - defp armed_by?(marker, attempt) do - case File.read(marker) do - {:ok, contents} -> match?([_vsn, ^attempt | _], String.split(contents, "\n")) - {:error, _unreadable} -> false + defp whose(contents, attempt) do + if match?([_vsn, ^attempt | _], String.split(contents, "\n")), do: :ours, else: :theirs + end + + defp removed(marker, deployment) do + case deployment.rm(marker) do + :ok -> :ok + {:error, :enoent} -> :ok + {:error, reason} -> {:stranded, unremovable(marker, reason)} end end + ## What a marker that could not be settled says + + # Why the marker is still there, and what that now means for the next start. + # + # The second half is the part an operator cannot work out for themselves, and it + # is the reason this is a failure rather than a log line: a marker at that path + # is a live instruction. If `prepare_restart_new_emulator/7` got as far as + # writing `new_start_erl.data` - which it does *before* the reboot, and which + # nothing ever removes - then the two files agree and the next ordinary start of + # this system boots the version this install failed to reach, with + # `:release_handler`'s own records calling it `unpacked`. That is precisely the + # state the marker protocol exists to make unreachable. + # + # The remedy is a removal, and it is named as one, because it is a thing the + # operator can do and Castle has just demonstrated it cannot: whatever stopped + # it - a mode, a read-only mount, a name that is no longer a regular file - is + # what there is to resolve. + defp stranded(rel_dir, why) do + "#{why} #{Path.join(rel_dir, @restart_marker)} is what tells the launcher to " <> + "boot a particular version on the next start, and #{@provisional_marker} - " <> + "which :release_handler writes before the reboot and never removes - may " <> + "already be beside it. Where it is, the two agree and the next ordinary " <> + "start of this system will boot the version this install did not finish, " <> + "which the release records will call unpacked. Remove the marker before " <> + "restarting this system, or do not restart it until that has been dealt " <> + "with. bin/castle releases will say where the system got to." + end + + defp unverifiable(marker, reason) do + "The restart marker this install armed cannot be accounted for: #{marker} " <> + "could not be read (#{:file.format_error(reason)}), so Castle cannot tell " <> + "whether it is still the one this attempt published - and it will not remove " <> + "a marker that may be a later attempt's, because that would take away a " <> + "reboot that is still owed." + end + + defp unremovable(marker, reason) do + "The restart marker this install armed is still there: #{marker} is this " <> + "attempt's and could not be removed (#{:file.format_error(reason)})." + end + ## What each way of failing to arm says defp pending(marker, refusal) do diff --git a/lib/castle/deployment.ex b/lib/castle/deployment.ex index 684a753..f7effb0 100644 --- a/lib/castle/deployment.ex +++ b/lib/castle/deployment.ex @@ -1,14 +1,27 @@ defmodule Castle.Deployment do @moduledoc false - # The two facts about the running deployment that say whether the release - # brought its own ERTS, and nothing else. They live here so that there is one - # place each is read - and so that a test can hand `Castle.Commands` a stub - # that answers them differently, which is the only way to reach a state - # `mix test` never runs in. The comparison itself stays in - # `Castle.Commands.ensure_own_erts/2`, so what a test exercises is the real - # rule over substituted inputs, the way the release-record check exercises the - # real rule over a substituted `which_releases/0`. + # The facts about the running deployment that Castle cannot arrange and a test + # cannot produce on demand. They live here so that there is one place each is + # read - and so that a test can hand `Castle.Commands` a stub that answers them + # differently, which is the only way to reach a state `mix test` never runs in. + # The rules built on them stay in `Castle.Commands`, so what a test exercises is + # the real rule over substituted inputs, the way the release-record check + # exercises the real rule over a substituted `which_releases/0`. + # + # Two roots, for the ERTS guard, and three filesystem operations: the `stat/1` + # that guard falls back to, and the `read/1` and `rm/1` that settle the restart + # marker's ownership on the way out of a failed install. All three are here for + # one reason - the answers that matter are the *failing* ones, and every way of + # arranging a failing `read` or `rm` from a fixture is a mode that root and some + # filesystems ignore. See `stat/1`. + # + # **This is not a general filesystem seam and must not become one.** The + # primitives that *publish* the marker - `Castle.Peer.work_dir/1`, + # `write_private/2` and `publish/2` - are deliberately called directly and not + # through here: what they guarantee is the point of them, and a stub would + # prove nothing about it. What these two carry is the opposite kind of thing, + # an outcome Castle has to have something to say about and no way to cause. @doc """ The root `:release_handler` resolves its own relative paths against. @@ -67,4 +80,33 @@ defmodule Castle.Deployment do """ @spec stat(Path.t()) :: {:ok, File.Stat.t()} | {:error, File.posix()} def stat(path), do: File.stat(path) + + @doc """ + Reads a file, for deciding whether the restart marker is still this attempt's. + + Here for the reason `stat/1` is, and the reason is sharper: the answer that + changes what Castle *says* is the one where the marker cannot be read at all, + and a marker Castle published is a regular file in a directory it verified it + could write to - so the only ways to make this fail are a mode applied + underneath it, a filesystem that went away, or a name that stopped being a + file. None of those is something a test may arrange and then rely on. + + An unreadable marker used to be treated as another attempt's and left where it + was, which is how an install could fail while leaving behind exactly the file + the next start acts on. Telling the two apart is what this exists for. + """ + @spec read(Path.t()) :: {:ok, binary()} | {:error, File.posix()} + def read(path), do: File.read(path) + + @doc """ + Removes a file, for clearing the restart marker this attempt armed. + + Here for the same reason as `read/1`. The argument this replaced was that a + removal could not fail where the publish had succeeded, since both need + permission on the same directory - which is true only if nothing changed in + between, and `install_release/1` runs in between and can take as long as an + upgrade takes. + """ + @spec rm(Path.t()) :: :ok | {:error, File.posix()} + def rm(path), do: File.rm(path) end diff --git a/test/castle/commands_test.exs b/test/castle/commands_test.exs index 0cb9f1f..a3c01db 100644 --- a/test/castle/commands_test.exs +++ b/test/castle/commands_test.exs @@ -26,10 +26,15 @@ defmodule Castle.CommandsTest do @tag :tmp_dir test "leaves nothing for install_release/1 to be given", %{tmp_dir: dir} do - # The order `Castle.install/1` composes these in, with the release handler - # ready to accept an install that must not be asked for. Everything able - # to refuse belongs on this side of the mutation, and a configuration that - # could not be materialised is the whole of what this adds to that list. + # `materialise/3` on its own, with the release handler ready to accept an + # install that must not be asked for. Everything able to refuse belongs on + # this side of the mutation, and a configuration that could not be + # materialised is the whole of what this adds to that list. + # + # This is the step's own contract rather than an ordering claim about + # `install/5`: the ordering inside the install is asserted where the install + # is, by the marker and configuration cases below. `Castle.commit/1` is the + # caller that still composes this in front of an operation. unpacked(dir) peer = PeerStub.stub({:error, "the compile environment does not agree"}) Stub.stub(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) @@ -159,12 +164,12 @@ defmodule Castle.CommandsTest do end end - describe "install/4" do + describe "install/5" do @tag :tmp_dir test "reports the version change", %{tmp_dir: dir} do handler = real_record(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) - assert Commands.install("1.2.3", dir, handler) == + assert Commands.install("1.2.3", dir, handler, configured(dir)) == {:ok, ["Now running 1.2.3 (previously 1.2.2)."]} assert Stub.calls(:install_release) == [[~c"1.2.3"]] @@ -174,7 +179,7 @@ defmodule Castle.CommandsTest do test "reports a restart of the emulator as the success it is", %{tmp_dir: dir} do handler = real_record(:install_release, {:continue_after_restart, ~c"1.2.2", ~c"upgrade"}) - assert {:ok, lines} = Commands.install("1.2.3", dir, handler) + assert {:ok, lines} = Commands.install("1.2.3", dir, handler, configured(dir)) assert Enum.join(lines, " ") =~ "Restarting to install 1.2.3 (previously 1.2.2)." end @@ -182,7 +187,7 @@ defmodule Castle.CommandsTest do test "reports a failure to install", %{tmp_dir: dir} do handler = real_record(:install_release, {:error, {:no_such_release, ~c"1.2.3"}}) - assert {:error, message} = Commands.install("1.2.3", dir, handler) + assert {:error, message} = Commands.install("1.2.3", dir, handler, configured(dir)) assert message =~ "Install of 1.2.3 failed." assert message =~ "no_such_release" end @@ -191,7 +196,7 @@ defmodule Castle.CommandsTest do test "reports a result it does not recognise", %{tmp_dir: dir} do handler = real_record(:install_release, {:whatever, ~c"1.2.2"}) - assert {:error, message} = Commands.install("1.2.3", dir, handler) + assert {:error, message} = Commands.install("1.2.3", dir, handler, configured(dir)) assert message =~ "Install of 1.2.3 returned an unexpected result." end @@ -204,7 +209,7 @@ defmodule Castle.CommandsTest do # to come first, and nothing may reach the handler. handler = synthesised_record(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) - assert {:error, message} = Commands.install("1.2.3", dir, handler) + assert {:error, message} = Commands.install("1.2.3", dir, handler, configured(dir)) assert message =~ "Cannot install 1.2.3: 1.2.2 is running from a release record" assert message =~ "running its old code" assert message =~ "the system has to be restarted" @@ -219,7 +224,7 @@ defmodule Castle.CommandsTest do # same reason the check lives inside the operation at all. handler = real_record(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) - assert {:ok, _} = Commands.install("1.2.3", dir, handler) + assert {:ok, _} = Commands.install("1.2.3", dir, handler, configured(dir)) assert Stub.calls(:which_releases) == [[]] end end @@ -239,7 +244,7 @@ defmodule Castle.CommandsTest do relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) handler = real_record(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) - assert {:ok, lines} = Commands.install("1.2.3", dir, handler) + assert {:ok, lines} = Commands.install("1.2.3", dir, handler, configured(dir)) # The version on the first line, which is all the hook reads, and the # attempt on the second, which is what makes the file this install's rather @@ -270,7 +275,7 @@ defmodule Castle.CommandsTest do relup!(dir, "1.2.2", [], [{~c"1.2.1", [], [:restart_emulator]}]) handler = real_record(:install_release, {:ok, ~c"1.2.2", ~c"downgrade"}) - assert {:ok, _} = Commands.install("1.2.1", dir, handler) + assert {:ok, _} = Commands.install("1.2.1", dir, handler, configured(dir, "1.2.1")) assert armed_version(dir) == "1.2.1" end @@ -280,7 +285,7 @@ defmodule Castle.CommandsTest do handler = real_record(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) assert {:ok, ["Now running 1.2.3 (previously 1.2.2)."]} = - Commands.install("1.2.3", dir, handler) + Commands.install("1.2.3", dir, handler, configured(dir)) refute File.exists?(marker(dir)) end @@ -296,7 +301,7 @@ defmodule Castle.CommandsTest do relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_new_emulator, {:apply, {:m, :f, []}}]}], []) handler = real_record(:install_release, {:continue_after_restart, ~c"1.2.2", ~c"upgrade"}) - assert {:ok, _} = Commands.install("1.2.3", dir, handler) + assert {:ok, _} = Commands.install("1.2.3", dir, handler, configured(dir)) refute File.exists?(marker(dir)) end @@ -307,7 +312,7 @@ defmodule Castle.CommandsTest do handler = real_record(:install_release, {:error, {:no_matching_relup, ~c"1.2.3", ~c"1.2.2"}}) - assert {:error, _} = Commands.install("1.2.3", dir, handler) + assert {:error, _} = Commands.install("1.2.3", dir, handler, configured(dir)) refute File.exists?(marker(dir)) end @@ -321,7 +326,7 @@ defmodule Castle.CommandsTest do relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) handler = real_record(:install_release, {:error, {:bad_relup_file, ~c"relup"}}) - assert {:error, message} = Commands.install("1.2.3", dir, handler) + assert {:error, message} = Commands.install("1.2.3", dir, handler, configured(dir)) assert message =~ "Install of 1.2.3 failed." refute File.exists?(marker(dir)) end @@ -331,7 +336,7 @@ defmodule Castle.CommandsTest do relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) handler = real_record(:install_release, {:whatever, ~c"1.2.2"}) - assert {:error, _} = Commands.install("1.2.3", dir, handler) + assert {:error, _} = Commands.install("1.2.3", dir, handler, configured(dir)) refute File.exists?(marker(dir)) end @@ -347,7 +352,7 @@ defmodule Castle.CommandsTest do File.mkdir!(marker(dir)) handler = real_record(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) - assert {:error, message} = Commands.install("1.2.3", dir, handler) + assert {:error, message} = Commands.install("1.2.3", dir, handler, configured(dir)) assert message =~ "Cannot install 1.2.3: #{marker(dir)}" assert message =~ "there is already a directory at that path" assert Stub.calls(:install_release) == [] @@ -364,7 +369,7 @@ defmodule Castle.CommandsTest do File.mkdir!(provisional(dir)) handler = real_record(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) - assert {:error, message} = Commands.install("1.2.3", dir, handler) + assert {:error, message} = Commands.install("1.2.3", dir, handler, configured(dir)) assert message =~ "Cannot install 1.2.3: the upgrade to 1.2.3 restarts the emulator" assert message =~ provisional(dir) assert message =~ "would pair with the marker this install is about to arm" @@ -377,7 +382,7 @@ defmodule Castle.CommandsTest do relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) handler = real_record(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) - assert {:error, _} = Commands.install("1.2.3", dir, handler, erts_less()) + assert {:error, _} = Commands.install("1.2.3", dir, handler, PeerStub, erts_less()) refute File.exists?(marker(dir)) end @@ -391,7 +396,7 @@ defmodule Castle.CommandsTest do relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) handler = real_record(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) - assert {:ok, _} = Commands.install("1.2.3", dir, handler) + assert {:ok, _} = Commands.install("1.2.3", dir, handler, configured(dir)) assert Path.wildcard(Path.join(dir, "castle-*")) == [marker(dir)] end end @@ -417,14 +422,14 @@ defmodule Castle.CommandsTest do relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) handler = real_record(:install_release, prepares_then_fails(dir, "1.2.3")) - assert {:error, _} = Commands.install("1.2.3", dir, handler) + assert {:error, _} = Commands.install("1.2.3", dir, handler, configured(dir)) refute File.exists?(marker(dir)), "the marker survived a failed install" assert File.exists?(provisional(dir)), "the fixture did not leave OTP's file behind" # The retry, which must not be able to pair with what the first left. Stub.stub(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) - assert {:ok, _} = Commands.install("1.2.3", dir, handler) + assert {:ok, _} = Commands.install("1.2.3", dir, handler, configured(dir)) assert armed_version(dir) == "1.2.3" refute File.exists?(provisional(dir)), @@ -450,7 +455,7 @@ defmodule Castle.CommandsTest do {:ok, ~c"1.2.2", ~c"upgrade"} end) - assert {:ok, _} = Commands.install("1.2.3", dir, handler) + assert {:ok, _} = Commands.install("1.2.3", dir, handler, configured(dir)) assert_received {:when_asked, true, false} end @@ -466,7 +471,7 @@ defmodule Castle.CommandsTest do File.write!(provisional(dir), "16.0 1.2.4") handler = real_record(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) - assert {:error, message} = Commands.install("1.2.3", dir, handler) + assert {:error, message} = Commands.install("1.2.3", dir, handler, configured(dir)) assert message =~ "a restart install is pending - it names 1.2.4" assert message =~ "consumed by the next start of this deployment" assert File.read!(marker(dir)) == "1.2.4\nsomeone-elses-attempt\n" @@ -493,12 +498,240 @@ defmodule Castle.CommandsTest do {:error, :whatever} end) - assert {:error, _} = Commands.install("1.2.3", dir, handler) + assert {:error, _} = Commands.install("1.2.3", dir, handler, configured(dir)) assert File.read!(marker(dir)) == foreign end end - # The three steps of `arm_restart/4` are one caller's sequence, and two callers + # Arming is one thing and *settling* is another, and the second used to happen + # only when `install_release/1` returned. An exit, a throw or a raise went past + # the two branches that disarmed, leaving the marker armed - and, where + # `prepare_restart_new_emulator/7` had already written `new_start_erl.data`, + # leaving the complete pair the launcher acts on. That is the "boots a version + # nothing installed" hazard the whole protocol exists to prevent, reached through + # the one path that does not return. + # + # The second half is that failing to settle it has to be *said*. A best-effort + # removal whose result nobody looks at, and an unreadable marker classified as + # somebody else's, both end with an operator meeting a stranded marker as a + # surprise boot. + describe "the restart marker, when the install does not return" do + @tag :tmp_dir + test "is settled when install_release/1 raises", %{tmp_dir: dir} do + # The discriminator, and it needs OTP's file written before the raise: with + # the marker left behind the two agree, and the next start boots 1.2.3 with + # the release records calling it unpacked. The exception itself is let out + # unchanged - Castle has nothing to add to it - so what this asserts is that + # it arrived *and* that the filesystem was settled on the way. + relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) + + handler = + real_record(:install_release, fn _args -> + File.write!(provisional(dir), "16.0 1.2.3") + raise "the relup blew up" + end) + + assert_raise RuntimeError, "the relup blew up", fn -> + Commands.install("1.2.3", dir, handler, configured(dir)) + end + + refute File.exists?(marker(dir)), "the marker survived an install that raised" + + assert File.exists?(provisional(dir)), + "the fixture did not leave OTP's half of the pair behind" + end + + @tag :tmp_dir + test "is settled when install_release/1 exits", %{tmp_dir: dir} do + # The other two classes go the same way. An `exit` is the one a real + # `:release_handler` produces most readily - a `gen_server` call to a process + # that went down while the upgrade was running. + relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) + handler = real_record(:install_release, fn _args -> exit(:killed) end) + + assert catch_exit(Commands.install("1.2.3", dir, handler, configured(dir))) == :killed + refute File.exists?(marker(dir)) + end + + @tag :tmp_dir + test "is settled when install_release/1 throws", %{tmp_dir: dir} do + relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) + handler = real_record(:install_release, fn _args -> throw(:no_matching_relup) end) + + assert catch_throw(Commands.install("1.2.3", dir, handler, configured(dir))) == + :no_matching_relup + + refute File.exists?(marker(dir)) + end + + @tag :tmp_dir + test "gives the serialised region up on the way out", %{tmp_dir: dir} do + # `:global.trans/3` is `try Fun() after del_lock(...)`, so an exception + # releases the lock rather than wedging every later install. That is + # `global`'s guarantee and not Castle's, and it is worth a test anyway: the + # re-raise is a *new* way of leaving the region, and the failure it would + # cause - every subsequent install blocking for ever - is not one this suite + # would otherwise notice. + # + # The second install has to come from another process. The lock is keyed on + # the requester, so two calls from the test process would be reentrant and + # this would pass against a leaked lock. + relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) + raising = real_record(:install_release, fn _args -> raise "the relup blew up" end) + + assert_raise RuntimeError, "the relup blew up", fn -> + Commands.install("1.2.3", dir, raising, configured(dir)) + end + + second = installer(dir, "1.2.2", as: :second, install: prepares_then_reboots(dir)) + + assert {{:ok, _}, [[~c"1.2.3"]], _} = Task.await(second, 30_000) + assert armed_version(dir) == "1.2.3" + end + + @tag :tmp_dir + test "does not disarm the marker a successful restart install left", %{tmp_dir: dir} do + # The reason this is `try/catch/else` and not `try/after`. An `after` cannot + # see which way the block went, so it would take away the marker whose whole + # purpose is to outlive this call - and the install would report a reboot + # that the next start had nothing to act on. + relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) + handler = real_record(:install_release, prepares_then_reboots(dir)) + + assert {:ok, _} = Commands.install("1.2.3", dir, handler, configured(dir)) + assert armed_version(dir) == "1.2.3" + assert File.exists?(provisional(dir)) + end + end + + # Being unable to settle the marker is an outcome Castle has to have something + # to say about and no way to cause, so it is reached through + # `Castle.Deployment.read/1` and `rm/1` - the seam `stat/1` established, and for + # the reason it gives. A fixture would need a mode, and root and some + # filesystems ignore modes, so it would only sometimes describe the state it + # names and would pass either way. + describe "the restart marker, when it cannot be settled" do + @tag :tmp_dir + test "says so when this attempt's marker cannot be removed", %{tmp_dir: dir} do + # A failed install that leaves the marker where it is, beside the + # `new_start_erl.data` its own preparation wrote. Silence here is an operator + # restarting a system whose install failed and getting the version it failed + # to reach. + relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) + handler = real_record(:install_release, prepares_then_fails(dir, "1.2.3")) + + assert {:error, message} = + Commands.install( + "1.2.3", + dir, + handler, + configured(dir), + unremovable_marker(dir) + ) + + # The failure the operator asked about is still reported - this is a second + # fact, not a replacement for the first. + assert message =~ "Install of 1.2.3 failed." + assert message =~ "bad_relup_file" + + # And then what it means. The marker, why it is still there, and what the + # next start will now do about it. + assert message =~ "The restart marker this install armed is still there" + assert message =~ marker(dir) + assert message =~ "could not be removed (permission denied)" + assert message =~ "the next ordinary start of this system will boot the version" + assert message =~ "Remove the marker before restarting this system" + + # The marker really is still there, so the message is not describing a state + # it also cleaned up. + assert armed_version(dir) == "1.2.3" + end + + @tag :tmp_dir + test "says so when it cannot tell whether the marker is its own", %{tmp_dir: dir} do + # An unreadable marker used to be classified as another attempt's and left + # alone, which reads as caution and is not: a marker that cannot be read is + # no evidence about whose it is, and the file it might be is the one the next + # start acts on. So it refuses to remove it *and* refuses to be quiet. + relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) + handler = real_record(:install_release, prepares_then_fails(dir, "1.2.3")) + + assert {:error, message} = + Commands.install( + "1.2.3", + dir, + handler, + configured(dir), + unreadable_marker(dir) + ) + + assert message =~ "Install of 1.2.3 failed." + assert message =~ "cannot be accounted for" + assert message =~ "could not be read (I/O error)" + assert message =~ "will not remove a marker that may be a later attempt's" + assert message =~ "Remove the marker before restarting this system" + end + + @tag :tmp_dir + test "says so, and says what happened, when the install raised as well", + %{tmp_dir: dir} do + # The two findings meeting. An exception is normally let out unchanged, + # because `Castle` is the boundary that raises and Castle has nothing to add + # to it - but a stranded marker is the thing an operator most needs told, and + # a stacktrace is where it would be buried. So this one branch reports + # instead, with the exception folded in rather than dropped. + relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) + + handler = + real_record(:install_release, fn _args -> + File.write!(provisional(dir), "16.0 1.2.3") + raise "the relup blew up" + end) + + assert {:error, message} = + Commands.install( + "1.2.3", + dir, + handler, + configured(dir), + unremovable_marker(dir) + ) + + assert message =~ + "Install of 1.2.3 raised, and the restart marker it armed could not be settled." + + assert message =~ "The restart marker this install armed is still there" + assert message =~ "Remove the marker before restarting this system" + + # The exception is not lost, and neither is where it came from. + assert message =~ "The failure itself:" + assert message =~ "the relup blew up" + assert message =~ "(RuntimeError)" + + assert armed_version(dir) == "1.2.3" + end + + @tag :tmp_dir + test "is quiet about a marker a start of the deployment consumed", %{tmp_dir: dir} do + # The ordinary case that must not be reported: any start or daemon of the + # deployment consumes the marker, so a failed install can perfectly well find + # nothing at the path. That is the outcome disarming wanted, not a failure to + # reach it - so `:enoent` is success, from both the read and the removal. + relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) + + handler = + real_record(:install_release, fn _args -> + File.rm!(marker(dir)) + {:error, {:bad_relup_file, ~c"relup"}} + end) + + assert {:error, message} = Commands.install("1.2.3", dir, handler, configured(dir)) + assert message == "Install of 1.2.3 failed. {:bad_relup_file, ~c\"relup\"}" + refute message =~ "restart marker" + end + end + + # The steps of the arming protocol are one caller's sequence, and two callers # can run it at once: `release_handler` serialises `install_release/1`, but that # is downstream of the read, the classification and the arming, so both callers # get past the marker check before either publishes. The loser then clears the @@ -528,12 +761,12 @@ defmodule Castle.CommandsTest do # The discriminator, and the whole of what the region buys. Without it the # second caller reads the running release, classifies the same transition - # and passes `unclaimed/2` here, while the first is still in front of its + # and passes `unclaimed/3` here, while the first is still in front of its # own arming - after which one of them destroys the other's evidence. refute_receive {:looked_up, :second}, 200 send(first.pid, :proceed) - assert {{:ok, lines}, [[~c"1.2.3"]]} = Task.await(first, 30_000) + assert {{:ok, lines}, [[~c"1.2.3"]], _} = Task.await(first, 30_000) assert Enum.join(lines, " ") =~ "The emulator is restarting." # Only now does the second caller look, and what it finds is a marker beside @@ -542,7 +775,7 @@ defmodule Castle.CommandsTest do # before as well; the difference is that it is now said about a pair that is # complete rather than said while taking half of it away. assert_receive {:looked_up, :second}, 10_000 - assert {{:error, message}, []} = Task.await(second, 30_000) + assert {{:error, message}, [], _} = Task.await(second, 30_000) assert message =~ "a restart install is pending - it names 1.2.3" assert armed_version(dir) == "1.2.3" @@ -551,6 +784,57 @@ defmodule Castle.CommandsTest do "the waiting caller cleared the reboot's own new_start_erl.data" end + @tag :tmp_dir + test "leaves the target configured by the one that installed it", %{tmp_dir: dir} do + # The defect this describes was not in the marker protocol at all: it was in + # `Castle.install/1` composing `materialise/3` and `install/4`, so two + # callers both materialised before either reached the lock. Materialising + # ends by renaming a resolved configuration onto the target's `sys.config` - + # a replace, necessarily, because that is the file `:release_handler` reads - + # so the loser's providers overwrote the configuration the winner's + # provisional release was about to boot, and the loser was then refused for + # the winner's marker. The install that was refused decided what the install + # that succeeded booted. + # + # 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. + relup!(dir, "1.2.3", [{~c"1.2.2", [], [:restart_emulator]}], []) + + first = + installer(dir, "1.2.2", + as: :first, + hold: true, + install: prepares_then_reboots(dir), + configure: configures("[{first, resolved}].\n") + ) + + assert_receive {:looked_up, :first} + + second = + installer(dir, "1.2.2", + as: :second, + configure: configures("[{second, resolved}].\n") + ) + + assert_receive {:started, :second} + refute_receive {:looked_up, :second}, 200 + + send(first.pid, :proceed) + assert {{:ok, _}, [[~c"1.2.3"]], [_configured]} = Task.await(first, 30_000) + + # The refused caller is refused *before* it materialises, which is the half + # that moving the materialisation inside the lock would not have fixed on its + # own: inside the region but ahead of `unclaimed/3`, it would still have + # replaced the configuration on its way to being told no. + assert {{:error, message}, [], []} = Task.await(second, 30_000) + assert message =~ "a restart install is pending - it names 1.2.3" + + # And the discriminator. The version waiting for its reboot holds the + # configuration of the install that armed it. + assert configuration(dir, "1.2.3") == "[{first, resolved}].\n" + end + @tag :tmp_dir test "hands the region on when the first install fails", %{tmp_dir: dir} do # The other direction, and what says the region is given up on every way @@ -567,8 +851,8 @@ defmodule Castle.CommandsTest do refute_receive {:looked_up, :second}, 200 send(first.pid, :proceed) - assert {{:error, _}, [[~c"1.2.3"]]} = Task.await(first, 30_000) - assert {{:ok, _}, [[~c"1.2.3"]]} = Task.await(second, 30_000) + assert {{:error, _}, [[~c"1.2.3"]], _} = Task.await(first, 30_000) + assert {{:ok, _}, [[~c"1.2.3"]], _} = Task.await(second, 30_000) assert armed_version(dir) == "1.2.3" end @@ -598,14 +882,14 @@ defmodule Castle.CommandsTest do refute_receive {:looked_up, :hot}, 200 send(restarting.pid, :proceed) - assert {{:ok, lines}, _} = Task.await(restarting, 30_000) + assert {{:ok, lines}, _, _} = Task.await(restarting, 30_000) assert Enum.join(lines, " ") =~ "The emulator is restarting." armed = File.read!(marker(dir)) # The hot caller arms nothing of its own, so it neither adopts nor disarms # the marker the restarting one left: the reboot that is still owed happens # on the version that asked for it. - assert {{:ok, ["Now running 1.2.3 (previously 1.2.2)."]}, _} = Task.await(hot, 30_000) + assert {{:ok, ["Now running 1.2.3 (previously 1.2.2)."]}, _, _} = Task.await(hot, 30_000) assert File.read!(marker(dir)) == armed end end @@ -813,7 +1097,7 @@ defmodule Castle.CommandsTest do test "refuses to install, without installing", %{tmp_dir: dir} do handler = real_record(:install_release, {:ok, ~c"1.2.2", ~c"upgrade"}) - assert {:error, message} = Commands.install("1.2.3", dir, handler, erts_less()) + assert {:error, message} = Commands.install("1.2.3", dir, handler, PeerStub, erts_less()) assert message =~ "Cannot install 1.2.3: the deployment and the emulator's root are different directories" @@ -925,42 +1209,52 @@ defmodule Castle.CommandsTest do # What `which_releases/0` reports for a node running `vsn` on a record it read # from a RELEASES file: it names applications, so the check `unpack/3` and - # `install/3` make passes. + # `install/5` make passes. defp running_record(vsn) do [{~c"sample", to_charlist(vsn), [~c"kernel-10.5", ~c"stdlib-7.2"], :permanent}] end # A handler whose running release was read from a RELEASES file, so it names - # applications and the check unpack/3 and install/3 make passes, with `fun` + # applications and the check unpack/3 and install/5 make passes, with `fun` # answering `reply`. defp real_record(fun, reply) do Stub.stub(:which_releases, running_record("1.2.2")) Stub.stub(fun, reply) end - # An `install/4` of 1.2.3 in a task of its own, from a node running `from`, + # An `install/5` of 1.2.3 in a task of its own, from a node running `from`, # reporting where it got to over messages so that two of them can be - # interleaved deterministically. It answers `{result, install_release calls}`, - # because both are per-process: `Castle.ReleaseHandlerStub` keeps its replies - # and its record of calls in the dictionary of whichever process called it, and - # here that is the task rather than the test. + # interleaved deterministically. It answers + # `{result, install_release calls, materialise calls}`, because all three are + # per-process: `Castle.ReleaseHandlerStub` and `Castle.PeerStub` keep their + # replies and their record of calls in the dictionary of whichever process + # called them, and here that is the task rather than the test. # # `hold: true` stops the caller inside `which_releases/0` until it is sent # `:proceed`. That is the seam the interleaving needs: the first thing the - # serialised region does and the last one before the marker is armed. + # serialised region does and the last one before anything is written. + # + # `configure:` is what this caller's peer does, and it is a reply rather than a + # fixed `{:ok, []}` so that a test can give two callers materialisations whose + # 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. 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, []}) Task.async(fn -> Stub.stub(:which_releases, lookup) Stub.stub(:install_release, reply) + PeerStub.stub(configure) send(test, {:started, name}) - {Commands.install("1.2.3", rel_dir, Stub), Stub.calls(:install_release)} + {Commands.install("1.2.3", rel_dir, Stub, PeerStub), Stub.calls(:install_release), + PeerStub.calls()} end) end @@ -996,11 +1290,75 @@ defmodule Castle.CommandsTest do # assert the wrong message while looking like it asserted the right one. defp erts_less, do: DeploymentStub.stub(System.tmp_dir!(), to_string(:code.root_dir())) + # A deployment whose ERTS guard is inert - no `RELEASE_ROOT`, which is what + # `mix test` runs in and what every case that omits this argument already gets - + # and whose filesystem refuses one operation on the marker. + # + # `nil` for both roots rather than the real ones, deliberately: `ensure_own_erts/2` + # returns early on a `release_root` of `nil` without asking for the other, so a + # `root_dir` that is never read is a `root_dir` this fixture is not claiming + # anything about. + # + # The refusal is scoped to the marker by path, so that nothing else these + # installs do is affected by it - the version directory, OTP's file and the + # working directory the marker is staged in all go through `File` directly. + defp unremovable_marker(rel_dir) do + DeploymentStub.stub(nil, nil) + DeploymentStub.stub_rm(refusing(marker(rel_dir), :eacces, &File.rm/1)) + end + + defp unreadable_marker(rel_dir) do + DeploymentStub.stub(nil, nil) + DeploymentStub.stub_read(refusing(marker(rel_dir), :eio, &File.read/1)) + end + + # `reason` for `path`, and the real operation for anything else. + defp refusing(path, reason, real) do + fn asked -> if asked == path, do: {:error, reason}, else: real.(asked) end + end + # Enough of an unpacked version directory for `materialise/3`: what is in it is # the peer's business, and the peer is a stub here. Everything it would look # for is covered against a real one in `Castle.PeerTest`. defp unpacked(dir), do: File.write!(Path.join(dir, "sys.config"), "[].\n") + # A target `install/5` can materialise: the version directory unpacked, which is + # what `materialise/3` looks for before it will reach a peer at all, and a peer + # that says it configured it. + # + # Every install case names one, and that is the point rather than an + # inconvenience. Materialising is a *step of the install* now, not something + # composed in front of it, so a case that did not say what the peer did would not + # have said what the version it installed is configured with - and that is + # exactly the thing the composition got wrong. + defp configured(rel_dir, vsn \\ "1.2.3") do + dir = Path.join(rel_dir, vsn) + File.mkdir_p!(dir) + unpacked(dir) + + PeerStub.stub({:ok, []}) + end + + # A peer that writes a distinguishable `sys.config` into the version directory, + # the way the real one ends by renaming a resolved configuration onto it. + # + # This is what makes "whose configuration is the version left holding" an + # observable question. `Castle.PeerStub` answering `{:ok, []}` cannot: the end + # state is the same whichever caller materialised, which is why no test saw the + # composition being wrong. + defp configures(contents) do + fn rel_vsn_dir -> + File.write!(Path.join(rel_vsn_dir, "sys.config"), contents) + {:ok, []} + end + end + + # What the target's `sys.config` says, which is whichever materialisation wrote + # it last. + defp configuration(rel_dir, vsn) do + rel_dir |> Path.join(vsn) |> Path.join("sys.config") |> File.read!() + end + defp marker(rel_dir), do: Path.join(rel_dir, "castle-restart-pending") defp provisional(rel_dir), do: Path.join(rel_dir, "new_start_erl.data") diff --git a/test/castle/erts_guard_test.exs b/test/castle/erts_guard_test.exs index 47750f3..3f1ec75 100644 --- a/test/castle/erts_guard_test.exs +++ b/test/castle/erts_guard_test.exs @@ -37,10 +37,18 @@ defmodule Castle.ErtsGuardTest do # :release_handler - which is the point. Without the guard, make_releases/0 # would find the Erlang installation's own releases/RELEASES and report # success, and remove/1 would be asking the handler to delete out of it. + # + # `install` names *itself* and `commit` names the configuration step, and + # that asymmetry is exact rather than untidy. `Castle.commit/1` still + # composes `materialise/3` in front of the operation, so the first refusal it + # meets is the configuration step's. `Castle.install/1` no longer composes + # anything: materialising moved inside `Commands.install/5`, behind that + # function's own guard, so what an operator asked for is what the refusal + # names. Losing "Cannot configure" here is the visible half of that move. refusals = [ {&Castle.make_releases/0, "Cannot create"}, {fn -> Castle.unpack("9.9.9") end, "Cannot unpack 9.9.9"}, - {fn -> Castle.install("9.9.9") end, "Cannot configure 9.9.9"}, + {fn -> Castle.install("9.9.9") end, "Cannot install 9.9.9"}, {fn -> Castle.commit("9.9.9") end, "Cannot configure 9.9.9"}, {fn -> Castle.remove("9.9.9") end, "Cannot remove 9.9.9"} ] diff --git a/test/support/deployment_stub.ex b/test/support/deployment_stub.ex index 918c22f..c701c49 100644 --- a/test/support/deployment_stub.ex +++ b/test/support/deployment_stub.ex @@ -29,22 +29,45 @@ defmodule Castle.DeploymentStub do two answers no fixture can produce on demand - a `stat` refused with `:eacces`, and a filesystem reporting no inode numbers - are reached at all. """ - def stub_stat(reply) do - Process.put({__MODULE__, :stat}, reply) - __MODULE__ - end + def stub_stat(reply), do: put(:stat, reply) + + @doc """ + Registers what the filesystem will say when the restart marker is read, or when + it is removed. + + Unregistered, both are the real thing, for the reason `stub_stat/1` is. They are + here because the answers that decide what `disarm/3` *says* are the failing + ones, and every fixture that produces a failing `read` or `rm` on a regular file + in a writable directory does it with a mode - which root and some filesystems + ignore, so the fixture would only sometimes describe the state it names, and + would pass either way. + + A reply that is a function of one argument is called with the path, which is + what lets a failure be made to bite on the marker alone. + """ + def stub_read(reply), do: put(:read, reply) + def stub_rm(reply), do: put(:rm, reply) def release_root, do: fetch(:release_root) def root_dir, do: fetch(:root_dir) - def stat(path) do - case Process.get({__MODULE__, :stat}, :unstubbed) do - :unstubbed -> File.stat(path) + def stat(path), do: filesystem(:stat, path, &File.stat/1) + def read(path), do: filesystem(:read, path, &File.read/1) + def rm(path), do: filesystem(:rm, path, &File.rm/1) + + defp filesystem(operation, path, real) do + case Process.get({__MODULE__, operation}, :unstubbed) do + :unstubbed -> real.(path) reply when is_function(reply, 1) -> reply.(path) reply -> reply end end + defp put(operation, reply) do + Process.put({__MODULE__, operation}, reply) + __MODULE__ + end + defp fetch(fact) do case Process.get({__MODULE__, fact}, :unstubbed) do :unstubbed -> raise "#{fact}/0 was called without a registered value" diff --git a/test/support/peer_stub.ex b/test/support/peer_stub.ex index 3e43d1b..1048e8d 100644 --- a/test/support/peer_stub.ex +++ b/test/support/peer_stub.ex @@ -12,6 +12,13 @@ defmodule Castle.PeerStub do @doc """ Registers the reply `materialise/1` answers with, and returns this module so that it can be passed straight to the function under test. + + A reply that is a function of one argument is called with the version directory + and its result used as the reply, the way `Castle.ReleaseHandlerStub` does it. + That is for the tests about what materialisation *left behind*: the real thing + ends by renaming a resolved configuration onto the target's `sys.config`, so a + stub that only answers `{:ok, []}` cannot show which caller's configuration a + version ended up with. One that writes a distinguishable `sys.config` can. """ def stub(reply) do Process.put(__MODULE__, reply) @@ -26,6 +33,7 @@ defmodule Castle.PeerStub do case Process.get(__MODULE__, :unstubbed) do :unstubbed -> raise "materialise/1 was called without a registered reply" + reply when is_function(reply, 1) -> reply.(rel_vsn_dir) reply -> reply end end From 6b299b64deb1c1f6474a2976f87c376f721e4f65 Mon Sep 17 00:00:00 2001 From: ausimian Date: Sun, 23 Aug 2026 19:44:50 +1000 Subject: [PATCH 5/6] 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 --- AGENTS.md | 26 ++++++++++++++++++++ RELEASE.md | 5 ++++ lib/castle.ex | 27 +++++++++++++++++++-- test/castle/commands_test.exs | 45 +++++++++++++++++++++++++++++++++-- 4 files changed, 99 insertions(+), 4 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index dfe5c96..602f8a6 100644 --- a/AGENTS.md +++ b/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 diff --git a/RELEASE.md b/RELEASE.md index f40e5ec..a490975 100644 --- a/RELEASE.md +++ b/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 diff --git a/lib/castle.ex b/lib/castle.ex index e9f40ff..b73138e 100644 --- a/lib/castle.ex +++ b/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/ 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 diff --git a/test/castle/commands_test.exs b/test/castle/commands_test.exs index a3c01db..37cfeda 100644 --- a/test/castle/commands_test.exs +++ b/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 From c0acc72e8876457174451078429d0fb334a2b87e Mon Sep 17 00:00:00 2001 From: ausimian Date: Sun, 23 Aug 2026 21:37:42 +1000 Subject: [PATCH 6/6] 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 --- AGENTS.md | 84 ++++++++++++++++++++------------- RELEASE.md | 13 +++-- lib/castle.ex | 57 ++++++++++++---------- lib/castle/commands.ex | 43 +++++++++++++++-- test/castle/commands_test.exs | 67 ++++++++++++++++++++++---- test/castle/erts_guard_test.exs | 20 ++++---- 6 files changed, 201 insertions(+), 83 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 602f8a6..9a9d9c2 100644 --- a/AGENTS.md +++ b/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)). diff --git a/RELEASE.md b/RELEASE.md index a490975..0024d2b 100644 --- a/RELEASE.md +++ b/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 - diff --git a/lib/castle.ex b/lib/castle.ex index b73138e..58d0063 100644 --- a/lib/castle.ex +++ b/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) diff --git a/lib/castle/commands.ex b/lib/castle/commands.ex index d4ef6c1..5dbf84e 100644 --- a/lib/castle/commands.ex +++ b/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)}"} diff --git a/test/castle/commands_test.exs b/test/castle/commands_test.exs index 37cfeda..9e109f7 100644 --- a/test/castle/commands_test.exs +++ b/test/castle/commands_test.exs @@ -1004,32 +1004,75 @@ defmodule Castle.CommandsTest do end end - describe "commit/3" do - test "reports what committing means" do + describe "commit/5" do + # Every case here names a `configured(dir)`, for the reason the install cases + # do: materialising is a step *of* the commit now rather than something + # composed in front of it at the boundary. It moved because the composition + # was racy - a duplicate install of the version being committed could + # materialise between the two calls, and its configuration would be what the + # newly permanent release booted. See `Castle.commit/1`. + @tag :tmp_dir + test "reports what committing means", %{tmp_dir: dir} do handler = Stub.stub(:make_permanent, :ok) + configured(dir) - assert Commands.commit("1.2.3", handler) == + assert Commands.commit("1.2.3", dir, handler, PeerStub) == {:ok, ["Committed 1.2.3. System restarts will now boot into this version."]} assert Stub.calls(:make_permanent) == [[~c"1.2.3"]] end - test "commits without asking whether the system can be upgraded from" do + @tag :tmp_dir + test "configures the version before making it permanent", %{tmp_dir: dir} do + # The ordering, asserted the way the install cases assert theirs: the + # handler stands ready to succeed, and what is checked is that the peer was + # asked first. A commit that made a version permanent and *then* configured + # it would leave the system permanently on a configuration nothing had + # resolved yet. + handler = Stub.stub(:make_permanent, :ok) + configured(dir) + + assert {:ok, _} = Commands.commit("1.2.3", dir, handler, PeerStub) + assert PeerStub.calls() == [Path.join(dir, "1.2.3")] + end + + @tag :tmp_dir + test "does not make it permanent if it cannot be configured", %{tmp_dir: dir} do + # And the other half: a configuration that cannot be resolved has to stop + # the commit, or the version becomes permanent with whatever was there + # before. + handler = Stub.stub(:make_permanent, :ok) + + File.mkdir_p!(Path.join(dir, "1.2.3")) + |> then(fn _ -> unpacked(Path.join(dir, "1.2.3")) end) + + PeerStub.stub({:error, "DATABASE_URL is not set"}) + + assert {:error, message} = Commands.commit("1.2.3", dir, handler, PeerStub) + assert message =~ "DATABASE_URL is not set" + assert Stub.calls(:make_permanent) == [] + end + + @tag :tmp_dir + test "commits without asking whether the system can be upgraded from", %{tmp_dir: dir} do # Deliberate, and not an omission. make_permanent/1 cannot write the # synthesised record back - do_make_permanent/2 returns early for a release # that is already permanent and errors for every other status - while a # refusal here would strand a version installed while the record was still # good, leaving the previous release to come back at the next restart. handler = synthesised_record(:make_permanent, :ok) + configured(dir) - assert {:ok, _} = Commands.commit("1.2.3", handler) + assert {:ok, _} = Commands.commit("1.2.3", dir, handler, PeerStub) assert Stub.calls(:which_releases) == [] end - test "reports a failure to commit" do + @tag :tmp_dir + test "reports a failure to commit", %{tmp_dir: dir} do handler = Stub.stub(:make_permanent, {:error, {:bad_status, :unpacked}}) + configured(dir) - assert {:error, message} = Commands.commit("1.2.3", handler) + assert {:error, message} = Commands.commit("1.2.3", dir, handler, PeerStub) assert message =~ "Commit of 1.2.3 failed." assert message =~ "bad_status" end @@ -1117,15 +1160,21 @@ defmodule Castle.CommandsTest do assert Stub.calls(:which_releases) == [] end - test "refuses to commit, without committing" do + test "refuses to commit, without committing or starting a peer" do + # `PeerStub` is unstubbed and raises if it is reached, so "refuses before it + # configures anything" is asserted by the guard holding rather than by a + # separate look - the same shape as the install case above, and it applies + # to `commit` now that materialising is a step inside it. handler = Stub.stub(:make_permanent, :ok) - assert {:error, message} = Commands.commit("1.2.3", handler, erts_less()) + assert {:error, message} = + Commands.commit("1.2.3", "/unused", handler, PeerStub, erts_less()) assert message =~ "Cannot commit 1.2.3: the deployment and the emulator's root are different directories" assert Stub.calls(:make_permanent) == [] + assert PeerStub.calls() == [] end test "refuses to remove, without removing" do diff --git a/test/castle/erts_guard_test.exs b/test/castle/erts_guard_test.exs index 3f1ec75..2caf7ed 100644 --- a/test/castle/erts_guard_test.exs +++ b/test/castle/erts_guard_test.exs @@ -38,18 +38,22 @@ defmodule Castle.ErtsGuardTest do # would find the Erlang installation's own releases/RELEASES and report # success, and remove/1 would be asking the handler to delete out of it. # - # `install` names *itself* and `commit` names the configuration step, and - # that asymmetry is exact rather than untidy. `Castle.commit/1` still - # composes `materialise/3` in front of the operation, so the first refusal it - # meets is the configuration step's. `Castle.install/1` no longer composes - # anything: materialising moved inside `Commands.install/5`, behind that - # function's own guard, so what an operator asked for is what the refusal - # names. Losing "Cannot configure" here is the visible half of that move. + # **Every command now names itself, and there is no asymmetry left to pin.** + # There was one: `commit` used to answer "Cannot configure", because + # `Castle.commit/1` composed `materialise/3` in front of the operation and + # the configuration step's guard was the first one it met. That composition + # is gone - it was racy, and `Commands.commit/5` materialises inside its own + # serialised region now, behind its own guard - so what an operator asked + # for is what the refusal names, for `commit` exactly as for `install`. + # + # This is the visible half of that move, and it is worth pinning in this + # direction rather than deleting: a "Cannot configure" reappearing here + # would mean a composition had come back at the boundary. refusals = [ {&Castle.make_releases/0, "Cannot create"}, {fn -> Castle.unpack("9.9.9") end, "Cannot unpack 9.9.9"}, {fn -> Castle.install("9.9.9") end, "Cannot install 9.9.9"}, - {fn -> Castle.commit("9.9.9") end, "Cannot configure 9.9.9"}, + {fn -> Castle.commit("9.9.9") end, "Cannot commit 9.9.9"}, {fn -> Castle.remove("9.9.9") end, "Cannot remove 9.9.9"} ]