Stop replacing the Mix release launcher; install Castle integration alongside it - #9
Merged
Conversation
`mix forecastle.relup` started `:sasl` but did not ensure it was on the build's code path. Elixir prunes OTP applications that the project does not depend on, so in any project that does not itself pull in `:sasl` - which is most of them, since Castle brings it in only for the release - the task failed with `:systools is not available`. Co-Authored-By: Claude <noreply@anthropic.com>
Forecastle replaced the Mix-generated `bin/<release>` launcher with its
own script, forking Mix's handling of cookies, distribution, VM args,
eval/rpc/remote, daemon mode and version selection. The fork had already
drifted: no recursive `readlink_f`, no `RUNTIME_CONFIG` handling, `eval`
dropping its trailing arguments, and a `--werl` flag Elixir no longer
has.
Leave the launcher to Mix and compose with it instead:
- `bin/castle` carries the release management commands, delegating to
the running system through `bin/<release> rpc`, so it inherits
cookie handling, distribution and node naming rather than repeating
them.
- The Castle boot integration is appended to the release's `env.sh`,
which the launcher sources with RELEASE_COMMAND already set and
before it resolves sys.config. An `env.sh` supplied by the project
through `rel/env.sh.eex` is preserved, and runs first.
- Version selection needs nothing at all: `release_handler` records
the committed version in `releases/start_erl.data`, which is where
the standard launcher already reads RELEASE_VSN from.
`bin/castle commit` may now be called without a version, committing
whichever release is running. There is at most one such release, so it
is unambiguous, and it saves naming the same version three times.
The preboot VM that expands `build.config` now runs from the release
root. `Castle.make_releases/0` resolves `releases/RELEASES` relative to
the working directory, so starting a release from anywhere else left the
system unable to manage its own releases.
Co-Authored-By: Claude <noreply@anthropic.com>
There was no test suite, which AGENTS.md rightly flagged as making a
green CI run weak evidence: the matrix was only a compile check, and
Forecastle's whole job is to shape a release correctly.
The suite builds a fixture application into a real release, in a
workspace under `_build/fixtures`:
- the step functions, against a synthetic `Mix.Release`;
- the assembled tree. The central assertion is that `bin/<name>` is
byte-identical to the launcher plain Mix produces, checked by
assembling the same fixture both ways rather than against a golden
file, so it keeps holding as Elixir's template evolves;
- `bin/castle` as a shell script, against a launcher stub that records
the arguments it was handed;
- booting a release and hot-upgrading it, asserting that the new code
is loaded, the process state survives, and the VM is not restarted.
The last of these is tagged `:e2e` and excluded by default. It runs
under `mix precommit` and on every cell of the CI matrix, which is where
the sensitivity to Elixir and OTP release tooling actually lives. The
fixture configures distribution without epmd, so it needs no daemon on
the host.
Co-Authored-By: Claude <noreply@anthropic.com>
`bin/castle` interpolated its version argument straight into the Elixir source it hands to `rpc`. `~s()` closes on the first unbalanced paren, so `1.2.3));System.stop(1)#` ends the sigil and the call, runs whatever follows on the node with the release cookie's authority, and comments out the tail. Reaching `bin/castle` already implies enough access to call `bin/<release> rpc` directly, so this buys an interactive caller nothing. It matters where the version is not typed by hand: `bin/castle install "$VERSION"` in a deployment script, with the version coming from CI metadata or an artifact name, turns that input into code execution. Validate against the characters a release version can be built from rather than escaping. The set is a superset of what `Version` accepts, so no version Mix can produce is turned away. The same sink was in `priv/script.sh` before it was removed, so this is carried-forward rather than new. The tests also caught that `commit` took its version without going through the check at all. Co-Authored-By: Claude <noreply@anthropic.com>
The launcher sources `env.sh` before it assigns `RELEASE_COOKIE`,
`RELEASE_NODE`, `RELEASE_TMP`, `RELEASE_MODE` and the rest. The Castle
integration starts the preboot VM from there, so `runtime.exs` was
evaluated with those unset - and reading `RELEASE_NODE` from runtime
configuration is ordinary, not exotic.
The launcher Forecastle used to generate assigned them before it ran the
preboot VM, and stock Mix has them set because its config provider runs
inside the booted VM. So this was a regression against both.
There is no hook that runs after the launcher initializes its
environment, so apply its defaults in the fragment instead, ahead of
expanding the configuration. Each uses the same `${VAR:-default}` form
the launcher uses and exports it, so the launcher's own assignments find
values already present and leave them alone: the ordering ends up the
same as stock, just reached earlier.
The fixture's runtime.exs now fetches these with `fetch_env!`, so a
regression fails the boot rather than quietly configuring the system
wrong.
Co-Authored-By: Claude <noreply@anthropic.com>
…etly Two ways a release could go wrong without saying so. Windows: configuration is withheld from Mix and expanded at boot by the env.sh integration, and there is no env.bat counterpart, so the .bat launcher looks for a sys.config nothing creates. `include_executables_for` defaults to `[:unix, :windows]`, so an untouched project assembles one. This is not new - sys.config was renamed unconditionally before this change too, and the launcher Forecastle used to generate was equally POSIX-only - but assembly succeeds either way, so warn. Not resolved by failing the assembly: the default includes `:windows`, so that would break every project that has not narrowed it, to prevent a breakage they may well not be exposed to. Real Windows support is a feature, not a fix, and belongs to its own issue. Empty commits: `bin/castle commit` with no version prints that there was nothing to commit and exited 0, so a deployment script could not tell the commit had not happened. It now exits non-zero. This is the one path here that Forecastle owns end to end; the wider problem, that Castle catches release_handler errors and returns :ok so every failed operation exits 0, needs Castle to report failures and is left for that. Co-Authored-By: Claude <noreply@anthropic.com>
An adversarial review raised two problems that cannot be fixed from this repo: Castle returns :ok for failed release_handler operations, so they exit 0, and Castle.generate/1 hardcodes its output path, so concurrent boots race on one sys.config. Both predate this branch. Filed as castle#15 and noted here so neither is rediscovered as new. Co-Authored-By: Claude <noreply@anthropic.com>
release_handler extracts with keep_old_files, so a hot upgrade never overwrites files already at the top level. A deployment built by an earlier Forecastle keeps its old bin/<release> across the upgrade, so it keeps the launcher this branch replaces, and the version argument handling fixed in fd32f1f never reaches it. bin/castle does appear, since new files are still extracted; both were confirmed against a real upgrade. Not worked around with a versioned CLI behind a top-level dispatcher: the dispatcher would be pinned in exactly the same way, and Mix's own bin/<release> has always had this property. Migration is an operator step, and now a documented one. Also stop the relup assertion in the e2e suite from being satisfiable by a stale file. mix forecastle.relup returns :error without a non-zero exit, so a failed generation is invisible to the helper; it now clears any earlier relup and checks the plan names both versions. Co-Authored-By: Claude <noreply@anthropic.com>
Two ways the composition diverged from the launcher it now defers to. The preboot VM ran from the release root, because make_releases/0 resolves releases/RELEASES relative to the working directory. That also moved everything else: a relative RELEASE_VM_ARGS, which the stock launcher and the old forked one both resolve from where they were invoked, resolved against the release root instead and the boot failed before it started. Any relative path the configuration itself reads moved with it. Only make_releases/0 needs the root, so it changes directory for itself through File.cd!/2 and the VM stays where the caller was. While there, the version and root now reach the expression through the environment instead of being interpolated into it, so a release root containing brackets cannot alter what is evaluated. bin/castle also resolved the launcher as bin/$RELEASE_NAME. RELEASE_NAME names the node; the executable is named at build time and does not move when it is set, so setting it sent the CLI after a launcher that does not exist. It is baked in now, as the tarball prefix already was, and RELEASE_NAME passes through the environment untouched. The test for that had copied bin/sample to bin/other first, which manufactured the file the bug needed and hid it. It no longer does. 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: efd3980873
ℹ️ 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".
The version allowlist was justified as a superset of what Version
accepts. That was wrong: Mix does not validate a release version at all.
`version: "1.0~rc1"` assembles and boots, and the allowlist then refused
every management command for it. `1.0)evil` and `1.0#{:x}` assemble too,
so the danger is real - it is the reach of the rule that was wrong, not
its existence.
Refuse what can end the ~s() sigil, escape inside it, or open an
interpolation - ( ) \ and # - plus the path separator, since the version
also names a tarball. Everything else is inert inside the sigil and now
goes through, including spaces, quotes, backticks and dollar signs: shell
parameter expansion is not rescanned, so none of them survive as
anything but text.
The accepted cases are no longer checked by comparing the expression
against a string. Each is parsed, and asserted to be a single call whose
one argument is a literal holding exactly that version - a <<>> of one
element, which is what makes an introduced interpolation a failure rather
than a different-looking string.
Co-Authored-By: Claude <noreply@anthropic.com>
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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.
Resolves #3.
Forecastle replaced the Mix-generated
bin/<release>launcher with its ownscript, forking Mix's handling of cookies, distribution, VM args,
eval/rpc/remote, daemon mode and version selection. The fork had alreadydrifted: no recursive
readlink_f, noRUNTIME_CONFIGhandling,evaldropping its trailing arguments, and a
--werlflag Elixir no longer has.This leaves the launcher to Mix and composes with it instead.
How it composes
bin/castlecarries the release management commands and delegatesthrough
bin/<release> rpc, so it inherits cookie handling, distribution andnode naming rather than repeating them.
env.shis extended, not replaced, with the fragment that expandsbuild.configintosys.configbefore boot. The launcher sources it withRELEASE_COMMANDalready set and before it resolvessys.config, which isexactly the window this needs. An
env.shsupplied throughrel/env.sh.eexsurvives and runs first.
release_handlerrecords thecommitted version in
releases/start_erl.data, which is where the standardlauncher already reads
RELEASE_VSNfrom.bin/castle commitmay be called without a version, committing whicheverrelease is running — there is at most one, so it is unambiguous.
Breaking
bin/<release> unpack|install|commit|remove|releaseshave moved tobin/castle. Deployments upgraded in place keep their old launcher — see theupgrade note in
RELEASE.md.Tests
There were none. The suite added here builds a fixture application into a real
release: the step functions against a synthetic
Mix.Release; the assembledtree, whose central assertion is that
bin/<release>is byte-identical to thelauncher plain Mix produces, checked by assembling the same fixture both ways so
it keeps holding as Elixir's template evolves;
bin/castleas a shell scriptagainst a launcher stub that records its arguments; and an
:e2esuite thatboots a release and hot-upgrades it, asserting new code is loaded, process state
survives and the VM is not restarted. Distribution runs without epmd, so no
daemon is needed on the host.
82 tests,
:e2eincluded inmix precommitand on every cell of the CI matrix.Review
Five rounds of adversarial Codex review, ending in
approve. Seven findingsfixed, each verified to fail without its fix where testable — including an RPC
injection through version arguments (
1.2.3));System.stop(1)#closed the sigiland ran on the node with the release cookie's authority) that also existed in
the launcher this removes.
Findings that were real but belong elsewhere are filed rather than folded in:
ausimian/castle#15, #6, #7, #8.
Also fixed
mix forecastle.relupfailed with:systools is not availablewherever theproject did not itself depend on
:sasl, because Elixir prunes unused OTPapplications from the build's code path.
RELEASESwas created relative to the working directory, so starting arelease from anywhere but its root left the system unable to manage itself.