fix: ask the node whether it can be upgraded - #16
Conversation
require_releases tested for releases/RELEASES on disk, which passes in exactly the case the gate exists to catch: a file that appeared after the boot that went looking for it. release_handler reads that file once, in its init, and otherwise works from a record it builds out of the boot script, which names no applications - so what is on disk says nothing about the record the running node holds, and only the node can tell the two apart. It now rpcs Castle.upgradable/0, which prints nothing when the running release can be upgraded from and raises Castle.Error when it cannot. The launcher exits non-zero, Castle's message reaches standard error with the remedy in it - a restart, the only thing that changes the record - and the status is the launcher's own, since a refusal and a node that could not be reached are different failures. unpack and install are both gated. install_release is the operation that acts on the record, and it is a separate invocation of this script: a restart, a RELEASES file that has gone away or gone unreadable, or a version staged by an older bin/<release> on a migrating deployment can all come between the two, so the answer given before the unpack does not carry. commit, remove and releases compare no records, and gating them would only add a way to refuse an upgrade already under way. Not through castle_rpc, which execs: a gate has work after it. In install the call is the last of the preflight - after the checks that need nothing from the system, and after the traps, so that a refusal still takes the capture directory with it - and before the launcher is asked to install anything, which is the rule that function is built around. Nothing has been installed when it refuses, so the epilogue has nothing to report. Every stub in the CLI suite answers the gate the way a system that can be upgraded answers it, so the bodies keyed on how many times they have been called still count only the calls they are about; CASTLE_STUB_NOT_UPGRADABLE is how a test asks for the refusal. The tests that pinned the file test are rewritten rather than extended: there is no file to write any more, and what they assert instead is who is asked, in what order, and what becomes of the install's report when the answer is no. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
Castle.make_releases/0 derives the releases directory from code:root_dir(), the root release_handler resolves its own relative paths against, so the call needs no working directory of its own. The File.cd! into a RELEASE_ROOT read out of the environment is gone and the eval is the bare Castle.make_releases(), which also leaves nothing about the release to be interpolated into the expression. The comment that explained the wrapper is corrected rather than deleted: why it was there - the file used to be resolved against the working directory, so it appeared wherever the launcher had been started from - is worth knowing. Everything else about the hook stands: guarded on the file's absence, start|start_iex|daemon|daemon_iex only, and a warning rather than a refusal when it cannot write. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
The release notes described a bin/castle unpack that refuses a system with no releases/RELEASES on disk. Both halves of that have moved: the question goes to the running system, through Castle.upgradable/0, and it is asked before an install as well as before an unpack. The entry now leads with what an operator has to know and could not otherwise work out - that the remedy is a restart and not creating the file, because release_handler never reads it again after its init and the first operation that changes anything writes the record it already holds straight back over it. The known limitation about testing for the file rather than asking the system is gone: it is no longer true. What replaces it is the limitation that remains - a system that cannot write the file can only be restarted, never repaired - keeping the account of what an upgrade from the synthesised record silently costs, which is why the gate is there at all. AGENTS.md gets require_upgradable in place of require_releases: what Castle answers, why the answer cannot come from the filesystem, which commands are gated and which are not, and where the call sits inside install's preflight. The closing rule stands as it was written - the moment the check needed to interrogate the node it became a Castle function - with the note that this is now what happened. The env.sh entry records that make_releases needs no working directory. README says both commands refuse, and that they ask the system. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
Castle 1.0 configures every version it installs in a peer and the branch that read a build.config is deleted along with the file, so there is no longer anything to dispatch on. Five passages still said there was, in the present tense, which is worse than saying too little: a reader has no way to tell they are describing history. README rested the matched-pair rule on the file. The rule stands - the two halves divide one job, Forecastle cannot constrain the Castle that brought it in, and nothing in the build can enforce the pairing - so what changes is what it rests on: a release assembled by a 1.x Forecastle carries the sys.config Mix wrote and nothing else, and an older Castle handed one looks for a file that is not there. AGENTS.md and lib/forecastle.ex gave the dispatch as the reason nothing here may write a build.config. The reason is now plainer: renaming what Mix wrote would leave the standard launcher with no configuration to boot from, and no Castle that reads the other name exists any more. The two test comments named the absent file as what selects the peer path. Nothing selects it; it is the only path. Both assertions are unchanged and now say what they are for - that the configuration is neither withheld nor written twice under two names, and that Castle resolves the version it installs in a VM booted on the preboot script pre_assemble/1 contributes. The release notes said the same of the atomic pair, and their migration note could be read as the new Castle expanding an old build.config. What expands it is the old version's own env.sh and its own copy of Castle, which the upgrade leaves in place; nothing in the new version reads that file. The rest of the two-world language is already past tense and correct, and the preboot script, which outlived the reason it was written, is still what Castle's peer boots. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7dfc064055
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # flat 1 - a refusal and a node that could not be reached are different failures, | ||
| # and whatever drove this may care which. | ||
| require_upgradable () { | ||
| if "$RELEASE_LAUNCHER" rpc "Castle.upgradable()"; then |
There was a problem hiding this comment.
When RELEASES has disappeared or become unreadable, the current node can pass this RPC using its valid in-memory record and then restart before the subsequent Castle.unpack/1 or Castle.install/1 RPC. The restarted node synthesizes an empty record, but the action proceeds without rechecking, recreating the silent stale-code failure this gate is intended to prevent. Move the check into the same Castle-side operation that performs the unpack or install so a restart cannot invalidate the answer between launcher calls.
AGENTS.md reference: AGENTS.md:L305-L309
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and already done — this is the fix, arriving as a pair, so the diff you are reading is the half that removes rather than the half that adds.
You have the failure exactly right, including the part that made it worse than "the answer is stale": the restarted node does not merely pass a check it should have failed, it then persists the synthesised record. do_unpack_release/4 ends in write_releases/3 over the records the handler holds, so the next boot reads the empty-libs record back as though it had always been there, and Castle.make_releases/0 does nothing because the file now exists. An unpack allowed through on a stale answer takes away the restart that the refusal would have named as the remedy.
Castle 22609f7 moved the check into Castle.Commands.ensure_upgradable/2, which unpack/2 and install/2 call themselves before :release_handler is asked for anything — same rpc, same node instance, and the same which_releases/0 result the action then proceeds on. This branch's 6245534 is the other half: the two separate Castle.upgradable() rpcs are gone from the preflight, with nothing replacing them. Net, it removes an rpc from the shell rather than adding one.
bin/castle upgradable survives at line 476, but it is now a subcommand an operator asks for, not a gate in front of anything — the state it reports is otherwise invisible, since the file can be present while the record the node works from was synthesised.
Gated unpack and install; deliberately not commit, remove or releases, which is measured rather than assumed: do_make_permanent/2 returns early for an already-permanent release and errors for every other status, do_remove_release/4 refuses the permanent release outright, and releases only reads — none can write that record back, while refusing them could strand an upgrade already installed and waiting to be committed.
One note on your AGENTS.md reference: you were reading it at the base. On this head that passage has been rewritten to record precisely this, and in its stronger form — that a question asked in one call and acted on in another is a question about a moment that has passed, so what the node holds is Castle's to know, and the decision goes with it.
The gate this repo put in front of unpack and install - one rpc to Castle.upgradable() before each - had the race it existed to prevent. Two rpcs are two moments and possibly two node instances: a node can answer on the record it read at boot, restart onto a synthesised one, and have the unpack or the install arrive afterwards and go ahead unchecked. An answer is only good for the call that acts on it. Castle now makes the check itself, inside unpack/1 and install/1, before :release_handler is asked for anything (castle d72e94e). So both rpcs go from bin/castle and nothing replaces them: the operations refuse for themselves, raise Castle.Error at the boundary as before, and the launcher exits non-zero with the reason and its remedy on standard error. require_upgradable goes with them. What is left in its place is a comment saying why there is nothing there, because the wrong shape is the one a reader would otherwise reach for. The refusal reads differently now: it leads with the operation that did not happen - "Cannot unpack sample-0.1.1: ..." - rather than with the state, so the suite expects it on the unpack and the install rather than on a call of its own. A refused install arrives after Castle has materialised the target's configuration, which starts a peer; that is harmless, since the peer writes only into the target's version directory, and it is not something to add a shell-side pre-check to hurry along. bin/castle upgradable is added, because the question is still worth asking deliberately: an operator who has just been refused, restarted, and wants to know where the system stands should not have to reach for bin/<release> rpc, which is the coupling bin/castle exists to remove. It is a plain command like releases - it gates nothing, and it says nothing when the answer is yes, which the usage text now spells out along with the fact that unpack and install ask for themselves. bin/castle is six lines shorter than it was before the gate was written. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6245534441
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| --boot "$REL_VSN_DIR/preboot" \ | ||
| --boot-var RELEASE_LIB "$RELEASE_ROOT/lib" \ | ||
| --eval "File.cd!(System.fetch_env!(~s(RELEASE_ROOT)), &Castle.make_releases/0)" || { | ||
| --eval "Castle.make_releases()" || { |
There was a problem hiding this comment.
When a consumer builds with Mix's supported include_erts: false, the generated releases/<vsn>/elixir runs the system erl, so :code.root_dir() identifies the shared Erlang installation rather than $RELEASE_ROOT (this reproduces on the supported Elixir 1.18 launcher). Consequently this call makes Castle target the system installation's releases/RELEASES: it normally fails due to permissions and leaves the deployment unable to upgrade, while a writable installation risks sharing release records across unrelated deployments; the local absence guard also invokes it again on every start. Castle needs an explicit way to derive the deployment root instead.
AGENTS.md reference: AGENTS.md:L137-L148
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, and thank you — this was the most valuable finding on the branch. Fixed on the Castle side, in ausimian/castle#24, now merged to release/1.0.0 as 21473b0.
You are right about the mechanism, and I confirmed each step from source rather than taking it on trust. Mix.Release.copy_erts/1 has a %{erts_source: nil} clause that copies nothing, and only the other clause writes the erl shim that rewrites ROOTDIR to the release root; Mix.Tasks.Release.replace_erts_bin/3 likewise only rewrites ERTS_BIN in releases/<vsn>/elixir when release.erts_source is truthy. So the launcher runs whichever erl is on the path and code:root_dir() is the shared installation. The earlier claim on this branch that code:root_dir() is RELEASE_ROOT in the preboot VM held only because copy_erts/1 runs when ERTS is bundled, which the default config and the e2e fixture both are — exactly the case that hides this.
It is also worse in practice than "usually fails for want of permission". A mise- or asdf-managed Erlang installation is user-writable and already ships its own releases/RELEASES, so make_releases/0 found that file, returned {:ok, []} and said nothing at all. Silent, on a developer box, by default.
Where I went a different way is the remedy. You suggested Castle needs an explicit way to derive the deployment root; deriving it from RELEASE_ROOT would be worse than the current failure. The root is :release_handler's own anchor, not Castle's choice — root_dir_relative_path/1 is filename:join(code:root_dir(), Pathname), and create_RELEASES/3 stores lib dirs relatively so the file stays relocatable. Castle writing to $RELEASE_ROOT would put the configuration and records where the handler never looks while the upgrade went on using applications under the installation: a silent divergence replacing a loud failure.
So there is no root Castle can pick that makes such a release upgradable, and it now refuses instead — Commands.ensure_own_erts/2, in front of make_releases, materialise, unpack, install, commit and remove, and deliberately not in front of upgradable/0 or releases/0, since gating a diagnostic on the condition it diagnoses leaves an operator nothing to ask.
Worth recording one correction to my own reasoning that four review rounds forced, because it bears on what the refusal may claim: the release records are not anchored to the root. init/1 takes the releases directory from {sasl, releases_dir}, then RELDIR, and only then init:get_argument(root). What cannot be relocated is the applications — extract_tar(Root, Tar), the relative lib/<app>-<vsn> entries, and the erts-<vsn> a removal deletes — which is why relocating the records is not a way out and why the guard is right regardless. That also surfaced a genuine pre-existing gap, filed as ausimian/castle#23: Castle ignores both overrides when deriving its own releases directory.
On the second half of your finding — "the local absence guard also invokes it again on every start" — I have deliberately not changed env.sh. It is true: RELEASES never appears on such a deployment, so the hook runs a preboot VM and takes the refusal on every start. A shell test for the condition would avoid that, and it is the wrong trade. A shell approximation of what the node knows is the class of bug this branch's own gate work removed, and a second implementation of the rule can drift from the first. One implementation, in Castle; the cost is a warning and a short-lived VM per boot of a deployment that could not have been upgraded either way. The reasoning is recorded in Castle's AGENTS.md so it does not get "fixed" later as an oversight.
What this branch does still owe is a test of the pairing — Castle refusing and this fragment continuing rather than treating it as fatal — which is boot-critical and belongs in the :e2e suite here, since it needs a real assembled release. Filed as #17. Not a 1.0.0 blocker, because Castle 1.0.0 depends on {:forecastle, "~> 1.0"}, so the older fragment that did exit 1 cannot be present in a fresh build.
Castle's mix.lock here was pinning a Forecastle three steps stale — stale enough that its vendored env.sh still reported that sys.config could not be generated, from a command this series deleted. That staleness is what made the refusal briefly look like it would stop such a deployment booting. Bumped.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6245534441
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| and version, which names no applications. Upgrading from that is silently wrong | ||
| rather than refused; see the fix below for what it leaves behind. | ||
|
|
||
| **The remedy is a restart, not creating the file.** Nothing can repair the |
There was a problem hiding this comment.
When releases/RELEASES exists but cannot be read, a restart alone cannot apply this remedy: priv/env.sh.eex guards creation with [ ! -f ... ], so the existing unreadable regular file causes every subsequent start to skip Castle.make_releases/0 and boot from another synthesized record. The operator must first make the file readable or remove it (and ensure it can be recreated), then restart; otherwise the newly added refusal leaves them in an endless restart loop.
AGENTS.md reference: AGENTS.md:L137-L139
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, and this is the third time on this branch series that a refusal has named more than it could deliver — so it is worth more than a doc tweak. Fixed on both sides.
Confirmed the mechanism rather than taking it on trust. release_handler:init/1 synthesises the record when RELEASES is missing or unreadable, and Castle's message already said as much — "was missing, or could not be read, when the system booted" — and then prescribed a single remedy for both. The hook here is guarded on absence, [ ! -f "$RELEASE_ROOT/releases/RELEASES" ], so a present-but-unreadable file is stepped over on every start: init/1 fails to read it again, synthesises another empty record, and the refusal repeats. An operator following that message restarts indefinitely. The diagnosis admitted two causes while the remedy covered one.
Castle's message now splits along the diagnosis it was already making — ausimian/castle#25. Absent, restart; present and unreadable, make it readable or remove it first, because the release will not replace a file that is there. Message-only: refusing is correct in both cases, and only the way out differed. Castle's AGENTS.md records why the two must not be collapsed back, since the tempting simplification is exactly the bug.
Here, 79610dd corrects the release note, which asserted both halves of the error — that the remedy is a restart, and that "what the refusal says is exactly that". It now names the unreadable case and points at Castle's message for which one applies.
Two things worth reporting from verifying this, since neither was visible from the diff:
The e2e suite had been passing against the wrong Castle. The fixture workspace is memoised, so its lock still pinned d72e94e — the release/1.0.0 head from before the ERTS guard merged. Clearing _build/fixtures and re-running resolved 21473b0 and gave 173 tests, 0 failures, which is the first time this suite has exercised a Castle carrying that guard, in a booted release through a real unpack-install-commit cycle. Worth knowing that a green e2e here does not by itself mean "green against current Castle".
The guard is inert on this fixture, as intended. It builds with ERTS included, so RELEASE_ROOT and code:root_dir() are the same directory and nothing is refused. That the pairing is not yet tested for an include_erts: false release — where Castle refuses and this fragment has to warn and carry on rather than treat it as fatal — is #17, filed rather than folded in, since it needs a fixture of its own.
The release note said the remedy for a synthesised release record is a restart, and that the refusal says exactly that. Both were true only for one of the two causes it had just named. The record is synthesised when releases/RELEASES was absent or unreadable, and the env.sh fragment creates the file guarded on [ ! -f ... ] - so a file that is present and cannot be read is stepped over on every start, and the system comes back on another synthesised record. Restarting on that advice never terminates. The note now splits the two. Castle's message does the same, in ausimian/castle#25, so an operator meeting the refusal is told which case they are in rather than having to read this. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Refs #6. Targets
release/1.0.0. The Forecastle half of Castle's step 3(
5c5e9e0), and the last piece of the castle#13 keystone.Ask the node, not the filesystem
bin/castlerefusedunpackwhenreleases/RELEASESwas absent, by testing for the filein shell. That is an approximation, and it passes in exactly the case the gate exists to
catch: a file that appeared after the boot that looked for it.
release_handlerreads itonce, in
init/1, so what matters is the record the running node holds — not what is ondisk now.
It now calls Castle's
upgradable/0, which refuses when the running release's applicationlist is empty and names a restart as the remedy.
Gated
unpackandinstall; notcommit,removeorreleases.installis theinteresting one:
install_release/1is the operation that folds over the running record(
get_new_libs(Current, New)), and it is a separate invocation of the script from theunpack — a restart between the two, a
RELEASESthat has since gone away or goneunreadable, or a version staged by an older
bin/<release>on a migrating deployment(root scripts do not update through a hot upgrade) all break the answer given earlier.
Note this only became worth doing because the test changed: with the old file check,
gating
installwas pointless, since after an unpack the file always exists. With therecord check it is the meaningful one.
commit,removeandreleasescompare no release records, so gating them could onlyadd a way to refuse an upgrade already under way. A test asserts they still work against a
system that has just refused, and that they never ask.
The gate uses the non-exec, status-capturing shape
install_and_confirmalreadyestablished —
castle_rpcexecs, and a gate has work after it. It sits last in thepreflight: after the local checks that need nothing from the node, and after the traps, so
a refusal still takes the capture directory with it.
install_outcomeis still emptythere, so the epilogue says nothing about an install that never happened —
asserted,
refute err =~ "outcome is not known".Create RELEASES without changing directory
Castle's
make_releases/0now derives the releases directory from:code.root_dir()— theroot
release_handlerresolves its own relative paths against — so theFile.cd!wrapper in
env.shis gone.--eval "Castle.make_releases()"is enough.The comment records why the wrapper existed rather than just dropping it: the file used to
be resolved against the working directory, so without it the file appeared wherever the
launcher happened to be started from, and landed in the release only by luck.
Everything else about the hook is unchanged — guarded on the file's absence,
start|start_iex|daemon|daemon_iexonly, warning and carrying on rather than refusing astart.
Stop describing a discriminator Castle no longer has
Castle 1.0 has one configuration path. The branch that read a
build.configis gone withthe file, so nothing selects anything any more — but five passages still said, in the
present tense, that Castle picks a path by whether that file exists: the README's "matched
pair" section,
AGENTS.md, a comment inlib/forecastle.ex, and comments in the assemblyand upgrade suites.
All five were corrected rather than cut, because each rests on something still true and
only the mechanism had to change. The README keeps its point — the halves divide one job,
Forecastle is a dependency of Castle and so cannot constrain it, nothing in the build can
enforce the pairing — and now rests it on what is actually true: an older Castle handed a
1.x release looks for a file that is not there and refuses the install.
RELEASE.md's migration note had a subtler version of the same error. It could be read asthe new Castle expanding an old
build.config; what actually expands it on a restartinto the old version is that version's own
env.shand its own copy of Castle, both leftin place by the upgrade. Nothing in the new version reads the old file.
A wider pass over the docs found nothing else: every other mention of the old arrangement
is already past tense and accurate.
Verification
mix precommitgreen including:e2e— 175 tests, 0 failures — on Elixir 1.19.5/OTP28.3 and again on 1.20.0/OTP 28.
The e2e suite drives real
bin/castle unpackandinstall, so both gates ran against thereal
Castle.upgradable/0. This is also the first run against a Castle with nobuild.configfallback present at all, rather than present but unreachable.