feat: install restart transitions under an external supervisor - #26
Merged
Conversation
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: ausimian/forecastle#10
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
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: ausimian/forecastle#10 Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
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: ausimian/forecastle#10 Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
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
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
Owner
Author
|
@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: 6b299b64de
ℹ️ 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".
Comment on lines
+120
to
+123
| `: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. |
There was a problem hiding this comment.
A fresh checkout still resolves Forecastle from mix.lock:7 at 3f32f70c..., the earlier pin that only picked up forecastle#4, so it does not contain the paired env.sh marker consumer/heart setup described here. Because mix deps.get honors that lock, releases built from this commit arm castle-restart-pending but retain the old launcher; under an external supervisor the restart install therefore still fails at heart:set_cmd/1 or cannot consume the marker on restart. Update the Forecastle lock to the paired change before advertising this path, since the repository explicitly states that the two halves are useless apart.
AGENTS.md reference: AGENTS.md:L600-L606
Useful? React with 👍 / 👎.
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 <noreply@anthropic.com>
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! 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 #14. Targets
release/1.0.0. Atomic with forecastle#19 — neither half works alone. Merge this one first.Restart transitions now install under an external supervisor, and the restart path runs end to end for the first time.
The blocker this removes
release_handlercallsheart:set_cmd/1while preparing any transition that restarts the emulator —prepare_restart_new_emulator/7, which both restart instructions go through, at two call sites. With noheartprocess that raisesbadarg, so the install fails beforeinit:reboot(). A Mix release under systemd normally runs without-heart.The fix is Forecastle's: run real OTP heart, deliberately defanged, with the supervisor as the only restart authority. Castle's half is the marker protocol and the serialization around it.
The marker is evidence about one install attempt
releases/castle-restart-pendingholds the target version and an attempt id.arm/4is ordered, and the order is load-bearing:new_start_erl.datasys.config.pristineisRefusing after clearing would silently take a concurrent install's reboot away. And after the clear,
new_start_erl.datacan only exist because this attempt wrote it — which is what lets "marker + OTP's file + versions agree" mean this attempt asked for the reboot. Version agreement alone could not carry that: a failed attempt to X leaves OTP's file behind, a retry to X arms a fresh marker, and a restart in between would have booted X though nothing installed it.disarm/3removes the marker only while the attempt id still matches, and answers four ways — ours (removed, or a failed removal reported), theirs (left, a reboot still owed), gone (quiet), unverifiable (reported). A stranded marker is an explicit failure, not a silent one, because an operator who is not told meets it as a surprise boot.The whole install is serialized, not just the arming
:global.transrestricted to[node()], around thewhich_releasesread, classification, materialisation, arming,install_release/1and disarm.global_name_serveris a kernel process, so this works on a non-distributed node — measured onnonode@nohostwithis_alive() == false: two callers, the second blocks until the first completes.Two races closed:
install_release/1— OTP's own lock — is too late. Two callers both pass the pending-marker check, the loser's clear destroys the winner's live evidence, and the winner's reboot falls back to the permanent release with a false "Nothing has been changed".sys.configover the first's on the way to being told no — and providers are not obliged to be idempotent, which is whysys.config.pristineexists. Moving it behind the refusals also means a refused install no longer materialises at all, retiring a costAGENTS.mdhad accepted.It also collapsed a bug that had nothing to do with races: the running release was being read twice, so classification and arming could reason from two different moments.
Exception-safe via an implicit
try/catch/else— notafter, which cannot see which way the block went and would disarm a successful restart install, destroying the marker whose whole purpose is to outlive the call. There is a test forbidding that.commit/1andrunning/1needed nothing, and that is measuredtransform_release/3writes thetmp_currentrecord back asunpackedon disk whileset_current/2makes itcurrentin memory, becauseinit:script_id()names it — exactly the statusdo_make_permanent/2accepts. Verified against OTP 27–29 source during review rather than assumed.The invariant that makes the residuals tolerable
Every remaining edge fails in the same direction — a lost or duplicated reboot, never a wrong release made permanent — because
make_permanent/1is the only writer ofstart_erl.dataand nothing in these paths touches it. Adversarial review tried to break that across four rounds and could not.Declared limitations
Castle.install/1against the same releases directory gets only the filesystem half. A filesystem lock would close it at the price of a stale lock wedging every later install after a hard kill.commit/1materialises outside the install lock, so committing X while installing the same X leaves two renames onto onesys.configunordered. Review confirmed this is not reachable by a routine install → crash → restart → install → commit sequence.disarm/3is check-then-unlink across two syscalls, and no POSIX call unlinks by identity. Fails toward a lost reboot.start_erl.datastill names the old version, so the wrong boot is one boot.Tests
112 → 140. Written to fail against what they name, and mutation-checked: reinstating the pre-lock materialisation fails the boundary case on the second caller's peer having been called; the exception and stranded-marker cases were each confirmed to fail against the pre-fix behaviour.
The concurrency tests barrier at
which_releases/0— the first thing inside the region and the last before arming. The destructive interleaving cannot be pinned by an end state even without the lock, because it needs a caller suspended between two adjacent statements with no seam, so the discriminator is that the second caller's lookup never happens:refute_receive, which can pass spuriously but never fail spuriously. Written down so the next reviewer does not hunt for an assertion that cannot exist.Castle.install/2..5mirrorsCommands.install/5so two concurrent callers can be driven through the public boundary; arity-1 is unchanged and nothing in a release passes the extras.Verification
mix precommitgreen across all six CI cells — Elixir 1.18×OTP27, 1.19×OTP27/28, 1.20×OTP27/28/29 — withcompile --warnings-as-errors. Four adversarial review rounds; fourteen findings across the pair, none declined.The restart path itself is exercised by forecastle#19's
:e2esuite, which drives unpack → install → reboot → provisional boot → rollback → commit against a real release. It also closed a documented gap here: the provisional boot is a cold boot of the materialisedsys.config, so a value changed between boots proves the target's own providers re-ran over it.