refactor!: leave runtime configuration to Mix - #15
Merged
Conversation
Forecastle intercepted all of it: :runtime_config_path set to false, a
substitute Config.Reader installed in Mix's place, every provider's init
argument rewritten into a keyword list with an :env key added, the
providers initialised at build time and stashed under the Forecastle key
in release.options, and the sys.config Mix wrote renamed to build.config
so the standard launcher could not boot from it. The env.sh fragment then
ran a preboot VM on every start, daemon and eval to expand it again.
That existed for one reason: the version being upgraded *to* needs a
configuration resolved by *its* providers, and a boot of the version
being upgraded *from* cannot produce one. castle#13 does that properly -
in a temporary peer, booted on the target's own code, running Elixir's
own pipeline - so the whole parallel implementation goes, and the two
defects it carried go with it rather than being repaired. Deleting it is
also what makes the new path reachable: Castle dispatches on whether
releases/<vsn>/build.config exists, so the file must stop existing.
initialize/1 goes too, though the issue does not name it. It seeded that
options key as an accumulator, and its only readers were the three
functions above.
The env.sh change cannot wait: the fragment called Castle.generate/1 on
every boot, inside a || { exit 1; }, and Commands.generate/1 reads
build.config unconditionally - so a release that stopped shipping that
file would stop booting. Castle.make_releases/0 still has to happen and
moves to bin/castle, ahead of unpack. unpack is the one door a new
version comes in through and the first operation that writes
release_handler's state out over that file, so it is the last moment the
file can be created; install, commit and remove all follow an unpack and
find it there. It runs on the running node, over the same rpc as every
other command, with the directory changed to the release root for the
call, because make_releases/0 resolves the file relative to the working
directory. bin/castle skips the launcher entirely once the file exists.
What is left in env.sh is a comment. That is deliberate: forecastle#10
needs somewhere to consume the provisional restart marker, and doing
nothing here on an ordinary start is what leaves room for it.
create_preboot_scripts/1 stays, for a different reason than it was
written: preboot is the script Castle's peer boots.
The Windows warning stays but its reason has changed, and it now says so.
The .bat launcher boots, because Mix writes the sys.config it reads; what
a Windows deployment has not got is bin/castle, which is POSIX shell.
Co-Authored-By: Claude <noreply@anthropic.com>
The two defects issue #6 opens with had no unit coverage that survived deleting the code, because what replaced that code is Mix. So they are pinned behaviourally instead, against a real release: the fixture is assembled as a project that names config/prod_runtime.exs through :runtime_config_path while config/runtime.exs sits beside it, and that declares three providers whose init arguments are a binary, a map and a list that is not a keyword list. Sample.EchoProvider returns whatever it was given, which is what makes the second observable at all: the terms can be read back out of the assembled sys.config and compared with what the project wrote. The first is asserted twice over - the copy Mix put in the version directory is the file that was asked for, and a booted VM says which file configured it, since both files set the same keys to different values. These pass because the interception is gone, not because it was corrected. There is nothing left in lib/ to unit-test here. Co-Authored-By: Claude <noreply@anthropic.com>
The release notes had three entries that this change makes wrong rather than merely incomplete, so they are corrected rather than added to. The Windows warning was justified by there being no env.bat counterpart for the boot-time expansion; the .bat launcher boots now, and what is missing is bin/castle. The release-variables fix was "the integration applies the launcher's defaults itself"; nothing expands configuration from env.sh any more, so the launcher exports them before the VM it configures even starts. The RELEASES fix was a File.cd! in the env hook, and is now one in bin/castle. The known limitation about concurrent boots racing on sys.config is resolved, not deferred: with Mix's pipeline restored nothing writes that file at boot. Two things are recorded as costs rather than glossed. A system that boots with no RELEASES file upgrades with the release record release_handler synthesises from its boot script, which has neither the application versions nor the ERTS version out of the .rel file - and since that file is read once, at startup, creating it from bin/castle cannot repair the system it runs on. And Castle.Peer refuses an unpacked Mix release, because release_handler copies <name>-<vsn>.rel into the version directory on every unpack (OTP-9746) while Mix's own copy there is called <name>.rel, so an unpacked release always holds two. That is what the :e2e suite found the first time it drove the peer path, it is one clause in Castle, and AGENTS.md says so rather than leaving the next reader to rediscover it - and says not to work it around here. Co-Authored-By: Claude <noreply@anthropic.com>
Creating it from bin/castle ahead of unpack achieved nothing at all, and the release note claiming a restart afterwards would fix it was wrong. release_handler reads releases/RELEASES in its init and otherwise builds a release record out of the boot script's name and version, with no applications in it. unpack then writes the record it is already holding straight back over the file - so the complete record bin/castle had just written was erased seconds later, and the restart read the erased one. Create, restart, then unpack is the only order that helps. So it goes back before the boot, which is the only moment it can be done, but guarded on the file's absence: the first start of a deployment pays for one short-lived VM and no start after it does anything. That is not the boot-time work #6 removed - no configuration is expanded, none of the launcher's defaults are applied, and eval is no longer covered, since an eval VM manages no releases. Failure warns instead of refusing, because a release does not need the file in order to run and a read-only release root is an ordinary way to run one. bin/castle keeps the gate rather than the creation: unpack refuses a system with no RELEASES and says to restart it first. The refusal is where it can be made without asking the node anything, so bin/castle also loses the live-node File.cd! it needed to create the file - a working directory is VM-global, and application processes doing relative file I/O during that window resolved against the release root. Testing the file instead of the node is exact for the case that arises and not for its converse; the note says so, and says where the complete test belongs. The e2e suite grows the case that was silently broken: :sample_dep, whose version moves with the sample's and whose appup asks for nothing, so no relup instruction loads its code. get_new_libs/2 is seeded from the running record, so an empty one means nothing compares as changed and the application is left reachable only through the superseded release's directory - which the next remove deletes. Against the previous commit it fails with sample_dep-0.1.0 after upgrading to 0.1.1. The relup task's failure message no longer names an application, since both of the fixture's now lack a path back and systools reports whichever it reaches first. Co-Authored-By: Claude <noreply@anthropic.com>
Both installation examples still asked for castle ~> 0.3.0, which cannot work with this Forecastle: Castle decides whether a release had its configuration intercepted at build time by whether the release has a build.config file, and this Forecastle no longer creates one, so an older Castle looks for a path it has not got and refuses the install. Saying it is the only enforcement available. Forecastle is a dependency *of* Castle, so it cannot constrain the version of Castle that brought it in, and Castle's requirement on Forecastle is the only constraint the build ever sees. Co-Authored-By: Claude <noreply@anthropic.com>
The known limitation said that restarting a deployment once after its first bin/castle command, or committing an upgrade and letting the next restart read the file, was enough to recover the complete release record. Neither is: unpack writes the record release_handler is already holding over the file first, so the restart reads what that wrote. Stating a mitigation that does not mitigate is worse than stating the limitation, so it is gone. What is left in its place is what remains true. A system that cannot write releases/RELEASES cannot be upgraded, the start says so at the time, and there is no repairing it while it runs. And bin/castle tests for the file rather than asking the node, which is exact for the case that arises and not for the converse, with the complete test named and placed. The silent upgrade defect is recorded as a fix rather than left implicit, because it is the part a consumer would otherwise discover from an application still running the code of a release they had removed. The env.sh and bin/castle entries follow the code: the hook creates the file on a first start rather than doing nothing, and unpack refuses rather than creating it. Castle's release-file defect comes out of AGENTS.md, fixed, and what replaces it is the fact worth keeping: an unpacked release holds two .rel files by OTP's design, and neither is to be tidied away. Co-Authored-By: Claude <noreply@anthropic.com>
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: 9c4188d89c
ℹ️ 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
+105
to
+107
| - The generated `env.sh` is extended with a hook that does nothing on a normal | ||
| start, and is reserved for consuming the provisional version marker that a | ||
| relup restarting the emulator leaves behind. Any `env.sh` the project |
There was a problem hiding this comment.
On a fresh deployment, this hook does not “do nothing”: priv/env.sh.eex launches the preboot VM to create releases/RELEASES, and failure produces a warning that prevents later bin/castle unpack. Describing it as merely reserved for a future restart marker hides both first-start overhead and the writable-release-root requirement from operators; document the one-shot bootstrap here instead.
AGENTS.md reference: AGENTS.md:L133-L150
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Fixed in 467b70a. You're right, and the README was simply inaccurate rather than merely thin — it said the hook "does nothing on a normal start" while on a first start it launches a preboot VM.
Both of the things you name as hidden are now stated: that the first start of a deployment pays for a short-lived VM, and that the release root has to be writable — with the consequence when it isn't, which is the part an operator most needs. The start still proceeds, with a warning, and bin/castle unpack later refuses rather than upgrading a system that cannot record what it is running. Every start after the first does nothing at all.
The marker-consuming role stays in the description, since that is what the hook is also for once forecastle#10 lands, but it is no longer the only thing said about it.
Worth noting why this got out of step: the hook genuinely did nothing at the point that sentence was written. The RELEASES bootstrap moved back into it later in this same branch, after it turned out that no post-boot placement can work — release_handler reads that file once, at startup, so creating it from bin/castle is erased by the next unpack before a restart can help. The prose describing the hook did not follow the code back.
It said the hook does nothing on a normal start and is reserved for the provisional version marker. On the first start of a deployment it creates releases/RELEASES, in a short-lived VM, which is what lets the system manage its own releases at all. Two things an operator needs and could not have read: that first start pays for a VM, and that the release root has to be writable — if it is not, the start proceeds with a warning and bin/castle unpack later refuses rather than upgrading a system that cannot record what it is running. Co-Authored-By: Claude <noreply@anthropic.com>
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 #6. Targets
release/1.0.0. The second and final task of step 2 of the pair withcastle#13, whose peer path this activates.
Read the issue's Scope update section for why this is a deletion rather than a repair.
The two defects it opens with are real —
:runtime_config_pathtreated as a boolean, andprovider arguments rewritten regardless of what they were — but they disappear
structurally once Forecastle stops intercepting configuration at all.
What Forecastle no longer does
Gone from
lib/forecastle.ex:remove_runtime_configuration/1,remove_config_providers/1,runtime_config_provider/1,add_config_providers/1,rename_sys_config/1,copy_runtime_exs/1,get_runtime_exs/0— andinitialize/1,which the issue does not mention but became dead once its only three readers went.
Mix's own pipeline is left in place. A release now has
sys.configwhere it used to havebuild.config, its providers are Mix's own in Mix's own order, andconfig/runtime.exsis expanded at boot by Elixir. Castle materialises the target's configuration during
install, in a peer running the target's own code — which is the point of the pair, sincea provider module can differ between the running version and the target.
create_preboot_scripts/1stays and is load-bearing for a new reason: it is the scriptCastle's peer boots.
The env hook change had to be here
Castle.generate/1readsbuild.configunconditionally, andenv.shcalled it at everyboot inside
|| { … exit 1; }. Deleting the file without removing the call would havestopped every release booting, so the hook change is in this commit rather than a later
one.
The hook now does nothing on a normal start except one guarded, one-off thing: create
releases/RELEASESif it is absent. That is not the boot-time work this removes — noconfiguration is expanded, no launcher defaults are applied,
evalis not covered, and afailure warns and carries on rather than refusing the start.
Why RELEASES is created at boot and not in
bin/castleIt was in
bin/castle unpackfirst, and that achieved nothing.release_handlerreadsRELEASES once, at startup, so a file created afterwards cannot repair the running
node — and
do_unpack_release/4then writes its in-memory state straight back over thenew file. The only order that helps is create → restart → unpack, so the create belongs
at the one moment that needs no restart.
Two alternatives were ruled out on evidence:
:saslto force a re-read.sasl.erl:183is{one_for_one, 0, 1}underOTP's own comment "Reboot node if release_handler crashes!", and
release_handlerispermanent — so zero restarts are allowed and killing it takes the node down. "Bounce
sasl" is a node restart, unannounced and at a moment nobody chose.
create_RELEASES/3passesRoot = ""and lib dirs are stored relative, explicitly sothe file is relocatable. What rules it out is that
systoolsnever writes RELEASES intoa tarball, and
create_RELEASESrecordsstatus = permanent: in an upgrade tarballthat is a false claim about the target, prevented from landing only by
keep_old_files. Resting the release history on one flag is a bad trade, and it woulddo nothing for deployments already in the field.
bin/castle unpacknow refuses when RELEASES is absent and names the restart as theremedy. That also removed the
File.cd!overrpcentirely: nothing inbin/castlemutates the live node's working directory any more, which it should never have done —
the working directory is VM-global, so any application doing relative file I/O during that
window resolved under the release root.
The file test is what the shell can express today; the exact gate belongs in Castle,
where
which_releases/0distinguishes a synthesised record (empty application list) froma real one. Recorded as such rather than left implicit.
The defect this fixes, which nobody had named
A system whose RELEASES was absent at first boot had no application versions in its
release record, so
code:replace_pathcovered only applications the relup explicitlyloads code for. An application whose version changed but whose code the relup never
touches stayed on the old code path after a successful upgrade — silently.
There is now an e2e test for exactly that: a second fixture application, versioned in
step, with an appup whose instruction lists are deliberately empty, asserting
:code.lib_dir/1moves across the install. Disabling either half of the fix leaves it onsample_dep-0.1.0after upgrading to0.1.1.Tests
170, 0 failures, including
:e2e— and this is the first time anything has drivenCastle's peer path end to end. It found one Castle bug on the way, fixed separately:
release_file/1demanded exactly one.reland an unpacked release always has two.The assembly assertions invert rather than disappear —
build.configabsent,sys.configpresent, the provider list read from Elixir'sconfig_provider_initandcompared field-for-field against a plain-Mix release, and
refute terms[:castle][:config_providers]. The env.sh test asserts the hook is present andinert.
New
test/forecastle/configuration_test.exs, with a fixture provider and a secondruntime config file: a project setting
runtime_config_path: "config/prod_runtime.exs"while
config/runtime.exsexists beside it gets the file it asked for — asserted twice,once on the copy Mix made and once on
bin/sample evalprinting a value only that filesets — and providers declared with a binary, a map and a non-keyword list reach
init/1with the term unchanged.
Docs
RELEASE.mdentries were corrected, not just added to: the Windows warning's statedreason (it was
env.bat/sys.config; it is now the absence ofbin/castle), theruntime-variables fix whose stated mechanism no longer exists, and the concurrent-boot
sys.configrace — which is resolved rather than deferred, since nothing writessys.configat boot any more.README now installs
~> 1.0in both examples, with a section saying the two packages area matched pair and why nothing can enforce it: Forecastle is a dependency of Castle and
cannot constrain the version that brought it in.
Verified on Elixir 1.19.5 and 1.20.0/OTP 28, full suite including
:e2eon both.