feat: boot the provisional version after an emulator restart - #19
Merged
Conversation
The Forecastle half of castle#14. Neither works alone.
Three things go into the release, all of them gated on a command that
starts the system.
The env.sh fragment selects the version an emulator-restart transition
installed. release_handler writes it to releases/new_start_erl.data and
leaves releases/start_erl.data - where the stock launcher reads
RELEASE_VSN from - naming the version that is still permanent, which is
the rollback property and worth keeping. Two markers are required, not
one: OTP's is written before the reboot and never removed, so on its own
it is not evidence that a reboot was asked for, and Castle arms one
beside it that says so. Both are consumed atomically, and before the
launcher is re-exec'd, so the second pass finds nothing.
It re-execs rather than assigning RELEASE_VSN in place. The launcher has
already resolved REL_VSN_DIR by the time it sources env.sh, and vm.args,
sys.config, the boot script and the elixir launcher itself all hang off
it.
The release now runs OTP's heart, configured to do nothing.
release_handler calls heart:set_cmd/1 while preparing the reboot and that
raises badarg with no heart process, so the install failed before
anything rebooted. HEART_COMMAND is unset, HEART_NO_KILL is TRUE, and
HEART_BEAT_TIMEOUT is at heart's documented maximum - because
HEART_NO_KILL alone does not make a heart-beat time-out harmless: the
port program exits after running its command, heart is a kernel process,
and init halts the node when one of those dies. -heart is added only if
it is not already there; two of them make init:get_argument(heart) answer
{ok, [[], []]}, which heart's own startup check has no clause for, and
the boot hangs.
bin/start is shipped and does nothing. It is the default start_prg path,
which release_handler composes into heart's temporary command and returns
unexamined, and heart really does run it - on init:reboot(), and on a
time-out where HEART_NO_KILL means the old VM is still alive. Starting
anything there would risk two live nodes.
mix forecastle.relup's auto strategy no longer refuses a restart edge.
settle_restarts!/2 is the whole verdict now: the all-hot line, or one
announcement naming both the edges classification chose and any
restart_emulator an appup asked for by name. refuse_chosen_restarts!/1,
refuse_restarts!/2 and describe_remedies/2 go with the refusal. The
two-stage refusal in appup_restarts!/1 is unchanged and stays
unconditional.
restart_upgrade_test.exs drives it end to end: a --restart relup through
unpack, install and commit, asserting the OS pid changes where the hot
suite asserts it does not, that an uncommitted provisional release rolls
back when it is killed, and that a commit makes it what an ordinary start
boots. The test is the supervisor, because nothing in the release is.
test/fixtures/sample/mix.exs points at Castle's issue branch for the
duration; it goes back to release/1.0.0 once both merge.
Refs: ausimian/castle#14
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
The env.sh fragment defaulted HEART_NO_KILL and HEART_BEAT_TIMEOUT and
only omitted HEART_COMMAND, so a deployment that already had any of them
in its environment kept active watchdog behaviour. An inherited
HEART_COMMAND is a second restart authority beside the supervisor,
HEART_NO_KILL=FALSE restores the kill, and a shorter HEART_BEAT_TIMEOUT
restores the death-by-time-out that the documented maximum exists to
make unreachable - while the README, this file's own comment and the
release notes all said the external supervisor was the only thing that
starts a replacement.
All three are assigned now, and HEART_COMMAND is unset. It overrides
rather than refuses: an operator who set one of these has a
configuration conflict, not an emergency, and a failed boot is a worse
answer than the conflict - so each displaced value is named on standard
error along with what replaced it and why, and a deployment that set
none of them says nothing at all. Each check is on the value rather
than on the variable being set, which is what keeps TRUE and an empty
HEART_COMMAND quiet, and what keeps the second pass of a provisional
start quiet too.
The test that covered this asserted the ${VAR:-default} expressions
were present, so it passed against exactly the defect above.
env_script_test.exs is what replaces it: the fragment is sourced in a
release-shaped directory with a stub launcher, and what is asserted is
the environment it leaves behind. The same harness covers the marker
half by observation rather than by reading the script - which version
each state of the two markers selects, and what a consumption
interrupted between the rename and OTP's removal leaves for the next
start. The restart e2e suite now runs the whole transition on a
deployment whose environment carries a HEART_COMMAND, a HEART_NO_KILL of
FALSE and an 11-second beat timeout, and asks the running node what it
was actually started with.
The claim that both markers are consumed atomically was false. The
rename that claims Castle's marker is atomic, so exactly one start can
act on the pair; OTP's file is then read and removed in separate steps,
and no POSIX operation moves two files together. The order is what
makes that safe - the marker goes first, so a start killed part way
through leaves no marker and the next start boots the version that was
permanent. The selection is lost, never duplicated. The claim file is
named per process now, so two racing starts cannot read each other's.
Castle's half of the same pair is castle#14: it clears any
new_start_erl.data an earlier attempt left before arming, publishes the
marker exclusively, and names the attempt on a second line. This side
still reads only the first line, which is the version.
Refs: ausimian/castle#14
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
Mix's generated elixir expands $ELIXIR_ERL_OPTIONS unquoted - set -- ...
$ELIXIR_ERL_OPTIONS $ERL "$@" - so what reaches the emulator is that
variable's fields under $IFS, and $IFS is space, tab and newline. The
guard was case " ${ELIXIR_ERL_OPTIONS:-} " in *" -heart "*, which is
bounded by literal spaces: an inherited -heart<TAB>-noshell is one word
that is not -heart, so a second -heart was appended and the emulator got
two. init:get_argument(heart) then answers {ok, [[], []]}, which
heart:check_start_heart/0 has no clause for, and the boot hangs with
nothing printed - measured against the assembled script, and the exact
failure the fragment's own comment describes. The fragment now iterates
over the unquoted expansion and compares each field, which is the
launcher's own splitting rather than an approximation of it.
Also: a heart variable that is set to nothing is a value, and
${VAR:-default} cannot see that. It treats set-and-empty as absent, so
HEART_NO_KILL= and HEART_BEAT_TIMEOUT= were being displaced in silence
while the release notes and this fragment's comments both promise to name
every value that stops taking effect. Both now read ${VAR-default},
without the colon, which distinguishes unset from set-and-empty and
reports the latter as []. An empty HEART_COMMAND stays silent, which is a
different rule rather than the same one: unsetting a variable that was
already empty changes nothing heart can read.
The coverage is what let both through. The unit test asserted the exact
string -heart, which was true of the one value it named and said nothing
about any other, so env_script_test.exs now counts how many -heart fields
the emulator would be given - a value with a newline in it cannot be
reported a line at a time anyway - across tab, newline and padded
values. The assembly test asserted the case expression, so it asserted
the defect; it now asserts the field comparison and refutes the string
match and the colon forms by shape.
No e2e start set ELIXIR_ERL_OPTIONS at all, because the fixture scrubs
it, so every real boot had the fragment assigning the flag rather than
finding one. The restart suite's first start now inherits a
tab-separated -heart, with -env CASTLE_TAB_PROBE tabbed behind the tab
so that the node answering with the value says the tabs really were
separators - without that, a boot proves only that nothing was done to
something that was never a flag.
Forecastle.Deployment.start!/2 puts a deadline on the launcher for the
same reason. That regression does not fail, it hangs, and it hangs inside
daemon rather than after it, because env.sh runs a preboot VM
synchronously on a first start and that VM inherits the same options.
System.cmd/3 has no deadline and setup_all has no ExUnit timeout, so
without it a regression stops the suite for as long as whatever runs it
will wait; measured by putting the old guard back, which had to be
killed.
One incidental correction, noted for whoever writes the next test here:
an empty value in System.cmd/3's :env removes the variable, the same as
nil does, so the test that claimed to cover an empty HEART_COMMAND was
covering the unset case. The empty ones are shell assignments prefixed
to the command now.
Refs: ausimian/castle#14
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
The guard that stops a second `-heart` being appended examined
`ELIXIR_ERL_OPTIONS` alone. That is the variable Mix's generated `elixir`
expands, and it is not the only path into the emulator's argument list:
`erlexec` prepends `ERL_AFLAGS` and appends `ERL_FLAGS` and then
`ERL_ZFLAGS` to the command line it builds. Each of the three puts
`-heart` into `init:get_argument(heart)` on its own - measured, one at a
time, each answering `{ok, [[]]}` - and with one of them set while the
fragment appended its own, `{ok, [[], []]}`, a `case_clause` in
`heart:check_start_heart/0` at heart.erl:348, and a boot that hangs
having printed nothing. So a deployment carrying the flag in any of the
three received the acknowledged silent hang.
Every test on either side of this passed against it, because every one of
them set the single variable the guard was reading - including the e2e
suite's hostile first start.
The guard now uses two loops: an outer one over the four variables with
each expansion quoted, so their values stay apart, and an inner one
unquoted, which is the launcher's own field splitting and unchanged from
before. `break 2` leaves both on the first match. `ERL_OTP<major>_FLAGS`
is deliberately not checked: `erlexec.c` calls it undocumented and for
OTP internal use, and its name carries a version the fragment would have
to start a VM to learn.
Coverage follows the same shape. `EnvScriptTest`'s flag counter now
counts across all four sources - a counter that read one variable is how
this stayed invisible, since it agreed with the guard - and there are
cases per source, per separator, and one for a source carrying other
flags but no `-heart`, which is what stops the fix being "never add one".
`RestartUpgradeTest`'s bounded first start inherits its tab-separated
`-heart` in `ERL_AFLAGS` rather than `ELIXIR_ERL_OPTIONS`, and asserts
that `ELIXIR_ERL_OPTIONS` is unset in the running node; against the old
guard that start hangs and the suite fails on its deadline, with the
stray preboot VM showing `-heart -- -noshell -heart`.
`Forecastle.Fixture` scrubs the three `ERL_*FLAGS` alongside
`ELIXIR_ERL_OPTIONS`. They matter more, not less: they are read by
`erlexec` rather than by anything Mix generates, so one set in a
developer's shell or a CI image would put a `-heart` into every start
every suite makes.
Also corrects this repo's note on Castle's install ordering, which said a
refused install materialises first and that this is harmless. Castle has
since folded that step inside its own lock, behind the refusals, because
materialising ends in a rename onto `sys.config`.
Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
The guard that stops a second `-heart` being appended read the environment
and worked out what the emulator would make of it. Three versions of that
shipped, and each was refuted by a narrower counterexample than the last:
* a match bounded by literal spaces missed `-heart<TAB>-noshell`, because
Mix's generated `elixir` expands `$ELIXIR_ERL_OPTIONS` unquoted and the
fields are separated under `$IFS`;
* splitting those fields, across all four flag variables, missed
`ERL_AFLAGS="'-heart'"`, `ERL_FLAGS='"-heart"'` and
`ERL_ZFLAGS='-he\art'` - `erlexec` applies shell-style quoting and
backslash escaping to everything it reads out of the environment, so all
three arrive as `-heart` while carrying no `-heart` substring at all;
* and a literal scan of `vm.args`, which the launcher passes as
`-args_file`, missed those same escapes, could not tell a commented flag
from a live one without a rule `erlexec` does not share (a `#` inside a
quoted value is not a comment), and could not see through a nested
`-args_file` at all.
Every one of those shipped with a unit test that agreed with it, because the
test mirrored the same model. Two `-heart` flags make
`init:get_argument(heart)` answer `{ok, [[], []]}`, which
`heart:check_start_heart/0` has no clause for - a `case_clause` at
heart.erl:348 - and the boot hangs having printed nothing.
So stop modelling. `erl -emu_args_exit` prints the argument vector `erlexec`
assembled, one argument per line, and exits without starting a VM. The
fragment runs that with the start's own environment, its own unquoted
`$ELIXIR_ERL_OPTIONS` and its own `-args_file`, in the order Mix's `elixir`
puts them, and looks for a line that is exactly `-heart`. That is not an
approximation of the union of the sources; it is the union, and it covers
`ERL_OTP<major>_FLAGS` too.
Four properties of the probe, each load bearing:
* it cannot hang, so it needs no timeout. A deployment already carrying two
flags of its own - the boot this exists to prevent - makes it print two
lines and exit 0.
* `-emu_args_exit` is undocumented: present in the `erlexec` binary in OTP
27, 28 and 29, absent from the usage string and the docs. It is relied on
because it fails safe, and the fragment makes that true rather than
assuming it - the probe carries a `-boot` naming a file that cannot exist,
and `init` takes the *first* `-boot` it is given, so an `erlexec` that
passed the flag through fails at once instead of starting a node; and the
answer is believed only if it contains the `-root` line every vector
carries. Both failures land in the same branch as an unreadable args file:
add nothing, and say so on standard error, because adding a flag that
turns out to be a second one hangs the boot in silence while adding none
fails loudly at `heart:set_cmd/1` with the system still running.
* an ordinary start pays nothing. The gate is the one remaining judgement
about text, and it is sound in the only direction that matters: a value
carrying none of `heart`, `args_file`, a quote, a backslash or a glob
character cannot become the token `-heart`, since the only transformations
applied are field splitting, pathname expansion and `erlexec`'s quote
removal. The args file is scanned with the `read` builtin, so no fork
either. A false positive costs one probe, ~11ms here.
* `ERL_OTP<major>_FLAGS` is covered by the probe and cannot trip the gate,
because POSIX sh has no way to enumerate variable names without a fork on
the common path. A deployment that sets only that variable is now
documented as unsupported and hanging. The previous note claimed the OTP
major could not be learned without starting the VM whose boot was at risk;
that was false, and is corrected where it appeared.
Coverage follows the same rule. `EnvScriptTest`'s flag counter no longer
re-implements the parse - it asks the same real `erlexec` - and the sandbox
holds a release's own `erts-*/bin/erl` that records every invocation, so
"no probe happened" is assertable, and a variant that strips
`-emu_args_exit` exercises the degraded path. The matrix covers quoted,
double-quoted, backslash-escaped and whitespace-separated values in each
variable and in `vm.args`, a nested args file both ways, a commented flag
(which is now correctly counted as absent), and an args file the emulator
refuses. The harness also runs the fragment under `set -e`, which is how the
launcher sources it - `bin/<name>` sets it before the `.` - so a top-level
non-zero exit can no longer pass here and kill a real boot.
`RestartUpgradeTest`'s fixture asks for its `-heart` spelled `-he\art`, at
assembly time so that only that suite's releases carry it, and the booted
node answers `{ok, [[]]}`. No reading of that file finds it.
Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
The probe that measures whether the emulator is already going to be
given a `-heart` had a gate in front of it: a `case` over
`ELIXIR_ERL_OPTIONS`, `ERL_AFLAGS`, `ERL_FLAGS` and `ERL_ZFLAGS`, and a
`read` loop over the args file, looking for `heart`, `args_file`, a
quote, a backslash or a glob character, so that an ordinary deployment
forked nothing at all. It is gone, and it should not come back.
Two reasons, and the second is a defect rather than a preference.
The gate was the last thing in the fragment reasoning about the *text*
of those values instead of measuring them, and its soundness rested on
a claim about `build_args_from_string()` in `erlexec.c` removing
nothing but quotes and backslashes. That is a claim about a C state
machine, of exactly the kind that was wrong three times before this
probe existed: a match bounded by literal spaces missed tabs and
newlines, then field splitting missed quoting and escaping, then none
of it saw an args file at all. Each shipped with a test that mirrored
the same model.
And it left a hole it could not close. `erlexec` prepends
`ERL_OTP<major>_FLAGS` exactly as it prepends `ERL_AFLAGS`, but that
name carries the emulator's OTP major and POSIX sh cannot enumerate
variable names without a fork of its own - so a deployment setting only
that variable never tripped the gate. A second `-heart` was appended,
`init:get_argument(heart)` answered `{ok, [[], []]}`, and the boot hung
having printed nothing. That was documented as unsupported. It is now
detected, because the question is asked unconditionally and `erlexec`
reads the variable.
What every start pays is one `fork`+`exec` of a C program that exits
without booting an emulator - 11ms, measured over 50 invocations - and
it is paid once per node start, because the whole fragment runs only
for `start`, `start_iex`, `daemon` and `daemon_iex`. An `eval` or an
`rpc` reaches none of it, which is what keeps the fork off
`bin/castle`'s path.
One thing is added that is not a gate: `-args_file` is passed only when
the path exists, via `${castle_args_file:+-args_file
"$castle_args_file"}`. `erlexec` refuses an args file it cannot open
and exits non-zero, which would otherwise make every start of a
release with no `vm.args` report the measurement as impossible and
decline to add the flag. This judges the filesystem rather than the
contents of anything: every variable `erlexec` reads still reaches the
probe, and a path that exists and cannot be read is still handed over
and still reported. The quoting inside the `:+` alternative was
measured across sh, dash, ksh and bash - a path carrying a space or a
glob character arrives as one unexpanded argument in all four.
Coverage inverts where the gate was the subject and holds where it was
not. "asks the emulator nothing at all" was a test *of* the gate, so it
is now "asks the emulator anyway" and asserts one invocation: the cost
of an ordinary start, asserted rather than tolerated. The five cases
whose flag variable or `vm.args` carried unremarkable flags assert one
probe instead of none, and the outcome each of them was about did not
move. The missing-args-file case additionally asserts the probe ran
*without* `-args_file`, which is the discriminator for the existence
check - without it that start takes the unmeasurable branch and the
release gets no heart at all. The `eval` case still asserts no probe,
because the fragment does not run for it, and that is now the only
assertion of its kind.
The new property is pinned directly: `ERL_OTP<major>_FLAGS=-heart`
alone must be detected, with the variable's name derived from the
running OTP major rather than written out - a hardcoded
`ERL_OTP28_FLAGS` is inert on four of the six cells this is verified
on, and a test that sets an inert variable measures nothing while
passing. Reinstating the gate fails nine tests, and this one fails with
the flag count at 2, which is the boot hang itself.
`Forecastle.Fixture` and `EnvScriptTest`'s own environment both scrub
`ERL_OTP<major>_FLAGS` now, appended at run time because its name
cannot be written into a module attribute. It leaks like the other
four, and it leaks into a fragment that now measures it, so one set in
a developer's shell or a CI image would hang an e2e boot rather than
merely add a flag nothing noticed.
`AGENTS.md`, `README.md`, `RELEASE.md`, `assembly_test.exs`, the
fragment's own comments and the fixture's `vm.args.eex` all claimed a
zero-cost common path and documented `ERL_OTP<major>_FLAGS` as
unsupported-and-hanging. Both were true of the gated version and
neither is true now. `-emu_args_exit` being undocumented, and relied on
because it fails safe, is unchanged and stays stated.
Refs: #10
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
The comment I wrote for it claimed a release with no vm.args is a supported shape. It is not one a stock build produces: mix release always renders rel/vm.args.eex into releases/<vsn>/vm.args, so the launcher's own RELEASE_VM_ARGS default always resolves to a file that exists. What the condition actually covers is a deployment exporting RELEASE_VM_ARGS to a missing path, or a hand-deleted vm.args - both starts the launcher fails on moments later with a message of erlexec's own. So it costs nothing and spares a doomed start a confusing warning, rather than holding up the common case. Corrected in the fragment, AGENTS.md and the test comment that repeated it. Co-Authored-By: Claude <noreply@anthropic.com>
Eleven assertions in castle_cli_test read File.exists?(record) to say the launcher was not reached. The root holding it was named with System.unique_integer/1 and created with File.mkdir_p!/1, which succeeds on a directory that is already there - and that integer is unique only within a VM run: three consecutive runs hand out 2690, 2693 and 2694, so the names recur. A run killed part-way leaves its argv behind, on_exit having never run, and a later run whose counter lands on the same value adopts it. Every one of those eleven assertions then fails for a reason that has nothing to do with the code. One such failure was seen on 1.20/OTP27 during matrix verification and did not recur on a re-run of the same cell with the same seed. The root is now removed before it is created as well as after, which is what makes it this run's own, and carries the operating system pid, which is what stops two VMs running at once choosing the same name and deleting each other's. on_exit cannot cover either case, since the one being defended against is precisely the one where on_exit did not run. Not reproduced on demand - it needs the earlier run to have died and the counter to collide - which is the argument for removing it by construction rather than chasing it. 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: 50d2e7326a
ℹ️ 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
+253
to
+255
| if [ -z "$castle_heart" ]; then | ||
| ELIXIR_ERL_OPTIONS="${ELIXIR_ERL_OPTIONS:+$ELIXIR_ERL_OPTIONS }-heart" | ||
| export ELIXIR_ERL_OPTIONS |
There was a problem hiding this comment.
When the target release adds -heart in its own vm.args but the permanent release does not, this first pass probes the permanent version's args and exports ELIXIR_ERL_OPTIONS=-heart; the later re-exec then probes the target args but preserves that inherited option, so the target boots with two -heart flags and hangs. Consume the provisional marker and re-exec before mutating the heart options so the guard measures only the version that will actually boot.
AGENTS.md reference: AGENTS.md:L353-L359
Useful? React with 👍 / 👎.
Comment on lines
+169
to
+172
| for castle_candidate in "$RELEASE_ROOT"/erts-*/bin/erl; do | ||
| if [ -x "$castle_candidate" ]; then | ||
| castle_erl="$castle_candidate" | ||
| fi |
There was a problem hiding this comment.
After an ERTS-changing release is unpacked, the release root can contain multiple erts-* directories, and this loop silently chooses the lexicographically last executable rather than the ERTS used by the selected release. On a downgrade, or with an older higher-numbered ERTS left in the root, the probe can therefore interpret a different ERL_OTP<major>_FLAGS variable from the real launcher, causing it either to append a duplicate -heart or omit the required one. Resolve the binary associated with the selected version instead of scanning the whole root.
AGENTS.md reference: AGENTS.md:L460-L465
Useful? React with 👍 / 👎.
The env.sh fragment re-execs the launcher to select a provisional
version, so it is read twice on the start that follows a restart-install
- and the boot is the second pass's. The heart block ran ahead of that
selection, so it decided about the version being replaced.
Pass 1 probed the permanent version's vm.args, found no -heart and
exported ELIXIR_ERL_OPTIONS=-heart. Pass 2 then probed the target's
vm.args with that variable inherited, measured the two flags together,
and so declined to append a third - but nothing removes what pass 1
exported. A target carrying its own -heart therefore booted with two,
which makes init:get_argument(heart) answer {ok, [[], []]}: the
case_clause in heart's own check_start_heart/0, and a boot that hangs
having printed nothing.
Measuring on both passes and appending on neither was never a fix for
that. The selection now comes first, so there is one pass that decides
anything and it is the pass that boots - with RELEASE_VSN and
REL_VSN_DIR naming the version the decision is about. An ordinary start
has no marker to consume and is unchanged; the re-exec still cannot
recur, because the marker is claimed by the rename before anything else
happens.
Nothing is exported across the re-exec beyond the version. One
consequence worth recording: a target release whose env.sh is not
Forecastle's now gets no heart configuration either, where before it
inherited pass 1's. That is right - Castle has no upgrade path into such
a release - but it is a change.
The suite had no case where the two versions' vm.args differed, which is
why this shipped. It has one now: a provisional start whose target
carries a -heart and whose permanent version does not, asserting that
one flag reaches the emulator and that it was asked once, about the
target. The stub launcher sources the fragment again so the second pass
is observable at all, and Forecastle.AssemblyTest pins the order in the
file that ships.
Co-Authored-By: Claude <noreply@anthropic.com>
The heart probe resolved its emulator by globbing "$RELEASE_ROOT"/erts-*/bin/erl and keeping the last executable match. A release root holds exactly one erts-* until an ERTS-changing release is unpacked into it, at which point the last match is whichever sorts last lexicographically - not even the newest, since erts-9.9 sorts after erts-16.2. ERL_OTP<major>_FLAGS is named for the OTP version of the binary that answers, and an args file may be written for one emulator and not another, so asking the wrong one is a question about a different deployment rather than a near miss. The launcher execs "$REL_VSN_DIR/elixir", and the only thing in that script choosing an emulator is its ERTS_BIN: mix release rewrites Mix's own ERTS_BIN="$ERTS_BIN" into ERTS_BIN="$SCRIPT_PATH"/../../erts-<vsn>/bin/ when the release brought an ERTS, and leaves it alone when it did not, in which case ERTS_BIN is empty and erl comes off PATH. That script's SCRIPT_PATH is the directory it is in, which is REL_VSN_DIR, so the assignment is read out of the same file the exec will read it out of and resolved against REL_VSN_DIR. Anything that does not come out executable falls back to erl, which is what an un-rewritten assignment means anyway - so include_erts: false keeps working, and so does a version directory that has lost its elixir. This is the resolution rather than a model of it, which is the same rule that put -emu_args_exit in front of the flag variables: nothing here reconstructs what the launcher would decide. start_erl.data's first field was the other candidate and is not usable. The unix launcher reads only field 2 - ERTS_VSN is set on Windows and never here - so a provisional start, which inherits RELEASE_VSN from the previous pass, would read the ERTS of the version being replaced. Mix also writes that field from the build machine's ERTS whether or not the release included one. Covered by a start where the lexicographically last erts-* is deliberately not the one the release names, by the include_erts: false shape, and by a provisional start whose two versions name different ERTS directories. A decoy emulator logs to its own file, since which binary answered is not visible in the arguments. The glob is refuted in the shipped env.sh, and the fragment's prose does not repeat how it was spelled, so that the refutation is about the code. Co-Authored-By: Claude <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4029d1dde7
ℹ️ 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".
|
|
||
| if mv "$castle_pending" "$castle_claim" 2>/dev/null; then | ||
| castle_armed=$(head -n 1 "$castle_claim" 2>/dev/null) || castle_armed="" | ||
| rm -f "$castle_claim" |
There was a problem hiding this comment.
When either marker cannot be removed—for example, if a corrupt castle-restart-consumed.$$ or new_start_erl.data path is a directory—the Mix-generated launcher’s set -e makes this unguarded rm -f terminate while sourcing env.sh. Because the pending marker has already been claimed, the documented mismatch path never warns and boots the permanent version; the service simply remains down. Treat cleanup failure as an unsettled pair and continue through the warning/fallback path.
AGENTS.md reference: AGENTS.md:L255-L259
Useful? React with 👍 / 👎.
The launcher sources this fragment under set -e, and rm -f still exits non-zero for a path it cannot unlink - a directory at the name being the easy case, since -f does not cover one. Both removals of the consumed markers were unguarded, so such a path ended the launcher where it stood, after the mv had already claimed the pending marker: no warning, no fallback to the permanent version, and no boot at all. A service that stays down is worse than one on the wrong version, and the wrong version was never the risk here. A removal that fails is now treated as this start having found the pair unsettled, which falls through to the mismatch path: it warns, execs nothing, and leaves the stock launcher to read start_erl.data. That is the permanent version, which is the direction the rollback property already rests on, and the leftover is then something an operator can see rather than something that stopped the system without saying so. The discriminator is the exit status rather than the output, because an unguarded rm ends the sourcing shell before the fragment can say anything: status non-zero and no warning at all. Mutation-checked - the new case fails with either guard removed. Co-Authored-By: Claude <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76449ee905
ℹ️ 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
+409
to
+411
| if printf '%s\n' "$castle_argv" | grep -q '^-root$'; then | ||
| if printf '%s\n' "$castle_argv" | grep -q '^-heart$'; then | ||
| castle_heart="measured" |
There was a problem hiding this comment.
When a valid vm.args uses -extra and has a plain argument literally named -heart, erl -emu_args_exit still prints that argument as its own -heart line, even though init:get_argument(heart) is error. This grep therefore suppresses the active flag Forecastle should inject, so the release boots without heart and a restart transition later fails at heart:set_cmd/1; only count -heart tokens that occur before -extra.
AGENTS.md reference: AGENTS.md:L373-L379
Useful? React with 👍 / 👎.
Everything after -extra is passed to the application rather than parsed as an
emulator flag, so a vm.args of "-extra\n-heart" leaves
init:get_argument(heart) answering error - measured - while -emu_args_exit
still prints a -heart line for it, because it prints the whole argument
vector. Counting that line suppressed the flag the fragment should have added.
Note which way that fails, because it is the opposite of every counting bug
before it and is why it outlived them: those added a second flag and hung the
boot with nothing printed, whereas this adds none. The release then boots
perfectly well without heart and the damage surfaces much later, as
heart:set_cmd/1 refusing a restart transition on a system that looks healthy.
This is not a return to modelling erlexec. The probe still asks it and reads
its answer; what is added is the boundary init itself applies to that answer.
Two test corrections came with it. The assembly suite asserted the literal
grep the probe used to run, which is a claim about spelling rather than about
behaviour; it now pins the -extra boundary instead. And its refutation of a
shell `exit 1` matched the awk program's own `{ exit 1 }`, so it is anchored
to a line whose whole content is that - what the test is actually about is the
fragment never taking the start down.
Mutation-checked: the new case fails against the unbounded count.
Co-Authored-By: Claude <noreply@anthropic.com>
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 #10, and lifts the
autorefusal added by #4. Targetsrelease/1.0.0. Atomic with castle#26 — merge that one first; this PR's fixture resolves against its branch.The launcher-side half of an emulator-restart upgrade: heart satisfied, the provisional version selected, and the stock launcher untouched.
heart, real and defanged
release_handlercallsheart:set_cmd/1while preparing any emulator restart, and with noheartprocess that raisesbadarg— so the install fails beforeinit:reboot(). Running real heart satisfies it through documented interfaces only. heart must then do nothing, because the supervisor is the only thing that starts this release:-heartviaELIXIR_ERL_OPTIONSheart:set_cmd/1returnsokHEART_COMMANDunsetHEART_NO_KILL=TRUESIGKILLa node that misses heartbeatsHEART_BEAT_TIMEOUT=65535$ROOT/bin/start, inertrelease_handlerinstallsTwo of those are pairs, and both pairings were measured rather than reasoned:
HEART_NO_KILLdoes not keep the node alive. It suppresses the kill, but the port program terminates once it has run the command, the Erlangheartprocess then exits{port_terminated, …}, andheartis a kernel process linked toinit— soinithalts the node anyway:Kernel pid terminated (heart). An earlier account of this was wrong because it observed aSIGSTOPed beam still alive; a stopped process cannot act on its own shutdown. Hence the maximum timeout.And
HEART_NO_KILLsuppresses the kill but not the command — heart still runssystem(cmd)on a heartbeat timeout, with the old node still up. Sobin/startdoing nothing is the whole point, not belt-and-braces.All three variables are assigned, not defaulted, and
HEART_COMMANDis unset rather than left alone. Defaulting them meant a deployment supplying any of them kept active watchdog behaviour while every document said it had none. Overriding is not refusing, though: each displaced value says so on stderr, and a deployment that set none — every ordinary one — says nothing.${VAR-default}without the colon, so set-and-empty is reported rather than silently treated as absent.The
-heartcheck asks erlexec instead of modelling itTwo
-heartflags makeinit:get_argument(heart)answer{ok, [[], []]}, whichcheck_start_heart/0has no clause for — acase_clauseatheart.erl:348, and the boot hangs having printed nothing. That matters because this fragment re-execs the launcher, so it is read twice on a provisional start.Three previous versions of this check modelled what the emulator would make of the environment, and each was refuted by a narrower case: matching between literal spaces missed
-heart<TAB>-noshell; splitting fields missedERL_AFLAGS="'-heart'",ERL_FLAGS='"-heart"'andERL_ZFLAGS='-he\art', all of which erlexec unquotes; and a literal scan ofvm.argsmissed both of those and a nested-args_file. Every one shipped with a unit test that agreed with it, because the test mirrored the same model.So the series is not continued.
erl -emu_args_exitprints the argument vector erlexec has assembled — applying its own quoting, escaping,#comments and nested args files — and exits without starting a VM. Six sources reach that vector and asking erlexec covers all six at once:ELIXIR_ERL_OPTIONS,ERL_OTP<major>_FLAGS,ERL_AFLAGS,ERL_FLAGS,ERL_ZFLAGS, and-args_file. It is not an approximation of the union; it is the union.There is deliberately no gate in front of it. There was one — a
caseover the variables and areadloop over the args file — and it was removed: it was the last thing here reasoning about the text of values rather than measuring them, resting on a claim aboutbuild_args_from_string()in erlexec.c, and it could not close theERL_OTP<major>_FLAGShole because POSIX sh cannot enumerate variable names without forking. Cost of removing it: one fork+exec of a C program that never boots an emulator, ~11ms, once per node start — the fragment never runs forevalorrpc.-emu_args_exitis undocumented, and is relied on because it fails safe rather than because it is guaranteed: the probe is believed only if the output carries a^-root$line, andinittakes the first-boot, so an emulator that passed the flag through cannot boot and exits without one. Either failure lands in the same branch — add nothing, and warn, naming the{ok, [[], []]}hang and theheart:set_cmd/1failure that is the cost of not adding one.The one remaining condition is a fact about the filesystem, not a judgement about contents:
-args_fileis passed only when the path exists. Defensive rather than load-bearing —mix releasealways rendersrel/vm.args.eex, so a stock build's default always resolves.Selecting the provisional version
releases/new_start_erl.dataplus Castle's correlation marker, both required.release_handlerreconcilestmp_currentback tounpackedon the next boot but never removes its own marker, andinit:restart/0keeps the same OS process soenv.shdoes not re-run — a stale marker survives until the next real OS restart. The claim is atomic (themvof Castle's marker, so exactly one start acts on the pair); OTP's file is read and removed after, so an interruption between them loses the selection rather than duplicating or misapplying it. Consumption is not atomic as a unit — POSIX has no multi-file rename — and the documents say that rather than claiming otherwise.Then
RELEASE_VSNis set and the stock launcher is re-exec'd, because merely assigning it after the launcher has computedREL_VSN_DIRdoes nothing.bin/<release>stays byte-identical to Mix's, still asserted.autono longer refuses a restart#4 shipped with
autorefusing any edge needing an emulator restart, because nothing could install one. That predicate is gone andannounce_restarts/2is live, withsettle_restarts!/2the single verdict over both the classified and the appup-supplied restarts.Tests
231 → 300, including a 23-case restart
:e2ecovering heart's state, the install, the VM restart, the provisional boot, marker consumption, rollback, and commit — asserting the OS pid changes, where the hot-upgrade test asserts it does not.Mutation-checked rather than asserted: reinstating the gate fails 9 tests, and the sharpest is the new
ERL_OTP<major>_FLAGScase, which fails withCASTLE_HEART_FLAGS == "2"— the gated fragment building the two-flag hang itself. Making-args_fileunconditional fails exactly one. TheERL_OTPvariable name is derived from:erlang.system_info(:otp_release)at run time, since a hardcoded one is inert on four of six cells.env_script_test.exssources the fragment against a stub launcher and asserts the effective environment, not the fragment's text — the weakness that let the earlier versions through. The real-boot probe inherits a tab-separatedERL_AFLAGS, and against the old guard it reproduces the hang exactly (-heart -- -noshell -heart), bounded by a deadline so a regression fails rather than hanging CI.Also fixed:
castle_cli_test's fixture root could adopt a killed run's leftovers —System.unique_integer/1is unique only within a VM run (three consecutive runs hand out 2690, 2693, 2694) andmkdir_p!succeeds on an existing directory, so elevenFile.exists?(record)assertions could fail for reasons unrelated to the code. One such failure was seen during matrix verification.Verification
mix precommitgreen including:e2eacross 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 over both diffs; fourteen findings, none declined.Follow-up owed
test/fixtures/sample/mix.exspins{:castle, github: "ausimian/castle", branch: "issue/14-restart"}so this PR's CI resolves against the Castle half. A follow-up commit onrelease/1.0.0must repoint it once both are merged — nothing enforces that, and the branch is deleted on merge.