fix: check the release record in the call that acts on it - #22
Merged
Conversation
upgradable/0 was a gate bin/castle asked for in an rpc of its own, before the rpc that unpacked or installed. Two rpcs are two moments and possibly two node instances: a node could answer on the strength of the record it read at boot, restart before the second call arrived, and synthesise an empty record on the way back up - and the unpack or the install would then go ahead on an answer that no longer held, which is the silent stale-code failure the gate exists to prevent. A check whose answer can be invalidated between asking and acting is not a check. So Commands.unpack/2 and Commands.install/2 make it themselves, before :release_handler is asked for anything. One call, one record, one decision, and nothing to reintroduce in the shell. install is checked because that is where the damage is. unpack is checked because it is the one other operation that writes release records: do_unpack_release/4 ends in write_releases/3 over the records the handler holds, so an unpack on such a node puts the synthesised record into RELEASES, the next boot reads it back, and make_releases/2 does nothing when the file is there - which takes away the restart the refusal names as the remedy and leaves the system with no way out. commit, remove and releases stay unchecked, and that is measured rather than assumed: do_make_permanent/2 returns early for a release that is already permanent and errors for every other status, do_remove_release/4 refuses the permanent release outright, and releases only reads, so none of them can write that record back, while refusing them could strand a version already installed and waiting to be committed. The refusal now names the operation that did not happen rather than reading as a precondition an operator failed to satisfy: "Cannot install 1.2.3: 1.2.2 is running from a release record OTP built from the boot script ...". upgradable/0 stays, as a query rather than a gate. Nothing has to call it, but the state it reports is invisible otherwise - the file can be present while the record the node works from was synthesised - so an operator needs some way to ask that does not unpack or install anything. Whether it belongs in the documented API surface is #11's. The new tests are about ordering, so they are written the way materialise/2's are: the stub is handed a reply that would have the operation succeed and the assertion is that it was never asked for it, because the refusal looks the same whether it came before the mutation or after. Two more assert which_releases was called in the call that acted, which no version asking it elsewhere can pass, and commit's guard is the mirror image - a synthesised record, and which_releases never called. Forecastle drops the separate upgradable rpc from bin/castle: nothing replaces it, because the operations refuse for themselves. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 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". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #13. Targets
release/1.0.0. Fixes a race in the gate step 3 added.The race
Step 3 added
Castle.upgradable/0, andbin/castlecalled it as a separate rpc beforeunpackand beforeinstall. Two rpcs are two moments and possibly two different nodeinstances: a node can pass the check on a valid in-memory record, restart before the
action's rpc with
RELEASESgone, synthesise an empty record, and then perform the actionwithout rechecking — recreating exactly the silent stale-code failure the gate exists to
prevent.
That was my design error rather than an implementation slip. A gate whose answer can go
stale before the action it guards does not reliably guard anything.
The fix
The check is now part of the operation.
Commands.unpack/2andCommands.install/2callit themselves, before
:release_handleris touched — same rpc, same node instance, and thesame
which_releases()result the action then proceeds on. It removes an rpc from theshell rather than adding anything.
Gating
unpackmatters more than it lookedThe original reason was "refuse as early as possible". The real one is worse, and is in
OTP's source:
do_unpack_release/4(sasl-4.3:1546-1547) builds its record list from thein-memory
Releasesand callswrite_releases/3, which (:2570-2580) only mapscurrent→
unpackedand otherwise serialises them verbatim.So an unpack on a synthesised-record node persists the empty-
libsrecord intoRELEASES. The next boot then consults it successfully — the synthesis path at:1259is no longer taken — and
Castle.make_releases/0does nothing, because the file exists.The node is left permanently refused, with the documented remedy, restart, no longer
working.
Letting
unpackthrough does not delay the refusal. It destroys the way out.commit,removeandreleasesstay unchecked — now measureddo_make_permanent/2returns early for an already-permanent release and errors for everyother status;
do_remove_release/4refuses the permanent release outright (:2109-2112);releasesonly reads. On a synthesised-record node the record names exactly one release,so none of them can reach a
write_releases/3. Meanwhile gating them could refuse anupgrade already under way — installed and awaiting commit — which the next restart would
roll back anyway.
Castle.upgradable/0stays, demoted to a queryThe state it reports is otherwise invisible: the file can be present while the record the
node works from was synthesised. Correctness no longer depends on anyone calling it, so
keeping it costs nothing, and #11 owns the documented API surface.
The
Castleboundary carries a comment saying it is a diagnostic and must not be putback in front of the operations. That comment is the durable protection against
reintroducing the race — more useful than deleting the function would be.
One consequence, documented rather than hidden
There is deliberately no second check in
Castle.install/1ahead ofmaterialise/1—that would be the shape just removed, moved in-process. So a node refused for its record
materialises the target's configuration first.
That is only wasted work: materialising writes into the target's version directory,
never to the running system and never to a release record, and it is idempotent. The
refusal still leaves the running system exactly as it was, and still falls before
install_release/1, which is the point of no returnAGENTS.mdrequires everything torefuse ahead of.
Messages name the operation that did not happen
with
Cannot unpack <name>/Cannot install <vsn>/This system cannot be upgradedsupplied per caller.
Tests
6 of 91 fail against
5c5e9e0— four behavioural, two textual. The behavioural fourare the two refusals and the two ordering assertions: each registers a reply that would
have the handler succeed, then asserts
Stub.calls(:unpack_release) == []/Stub.calls(:install_release) == [], since the refusal value alone cannot distinguish"before" from "after".
commit/2 commits without askingpasses against5c5e9e0by design — it guards thedecision not to gate commit rather than discriminating against the old code, and says so.
Verification
mix precommitgreen — 91 tests — on Elixir 1.19.5/OTP 28 and again on 1.20.0/OTP 28.Forecastle follows
Its side is a deletion: the two separate
Castle.upgradable()rpcs go, with nothingreplacing them. Landing separately, since its current suite expects them.