feat: expand target configuration in a peer of its own (castle#13, step 1 of 3) - #18
Conversation
Installing or committing a version expanded that version's configuration by folding provider state stashed at build time, in whichever node happens to be running. That is the wrong answer as soon as a provider module differs between the running version and the target - which is exactly what an upgrade can change - and it means Castle carries a second implementation of a pipeline Elixir already has. The target's configuration is now expanded by running the target's own providers, in a peer booted from the target's own preboot script on the emulator its release file names. The peer connects over standard_io, so no epmd, cookie, node name or distributed Erlang is involved, and what it prints reaches whoever asked for the install. Elixir's Config.Provider.boot/1 is what folds the providers; nothing here iterates over them, which is the whole point. What is arranged around that call is only that the pipeline write its result rather than configure the VM it is running in, since that VM is thrown away. Which of the two ways applies is settled by the release itself. build.config exists exactly when Forecastle intercepted the configuration at assembly time, so a release assembled by today's Forecastle keeps the path it has always taken and nothing observable about installing or committing it changes. The peer path activates by itself once Forecastle stops intercepting, which is the second of the three steps this lands in. The test is the presence of build.config rather than the absence of sys.config, because such a release has both from its first boot onwards. The peer is started linked, so it cannot outlive the command, stopped on every path out including the failing ones, and bounded at both its boot and its call - a peer that never answers cannot hold an install open. Everything able to refuse refuses before install_release/1 is asked for anything: a missing boot script, an absent emulator, a provider that raises. And the resolved configuration is assembled in a copy beside sys.config and renamed onto it, so no version is left holding half a configuration. Standard error inside the peer is relayed through its user process. A standard_io connection multiplexes the peer's console output with its own framing and reserves sixteen byte values for it, all of them UTF-8 lead bytes, so a provider writing an accented character straight out would fail the frame's checksum, take the channel down, and refuse an install that was about to succeed. Peers are tested by starting real ones, against a release-shaped tree built by systools rather than by mix release: stubbing the peer would prove nothing about the one thing it exists to do, which is to run a release's own code. The test that matters most compiles two versions of one provider module, loads one into the node running the tests and puts the other on the peer's code path, and asserts the answer came from the peer's. Refs: #13 Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
Both files described one way of expanding a target's configuration. There are two now, so the discriminator, why it is sound, and what each side of it does are written down - along with the four properties of the peer that are load-bearing rather than incidental: linked and stopped on every path, bounded at boot and at call, refusing before install_release/1, and written by rename. Two limitations are stated rather than left to be discovered. The concurrent-boot race on sys.config does not go away here: the boot path still runs generate/1 until Forecastle stops intercepting and the third step deletes it. And the materialised configuration becomes the base the next materialisation resolves over, because it is written to the file release_handler reads - the same answer for a provider that sets what it cares about, a lingering value for one that sets it conditionally. How that file and a later cold boot of the same version interact only becomes reachable with forecastle#6, and belongs there. The release note says what changes for a consumer, which for a release assembled by the Forecastle this ships beside is nothing at all. Refs: #13 Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
Two findings from review of the peer, both in the same functions, so both here. Elixir's compile-environment check was never made. It refuses to boot a release whose resolved configuration contradicts what Application.compile_env/3 read when the release was compiled, and it makes that check in the branch that applies a configuration, or on the boot that follows the branch that writes one. Asking the pipeline to write and replacing the reboot with a function that returns leaves neither, so a release Elixir considers unbootable passed materialisation and reached install_release/1 - and for an upgrade that restarts, the problem then surfaces on the way back up, where a rollback is the only way out. The check is now made with Elixir's own validator, in the way Elixir makes it: the resolved values of the applications the check names are put into the peer's application environment, persistently so that loading an application cannot overwrite them from its .app file, and Config.Provider.validate_compile_env/1 is asked. Anything other than a list of triples or an explicit "off" is refused rather than skipped, because a check that quietly passes everything is indistinguishable from one that works. The tests that cover it build the provider state through Config.Provider.init/3, so a change in how Elixir represents the check takes the refusal with it and fails them. Elixir's other boot-time check, that a configuration does not configure kernel or stdlib too late, is deliberately not reproduced: that is about whether a configuration can be applied to a VM already running, which is a property of a boot, and the target makes it when it boots. The control connection is now a socket on the loopback interface rather than standard_io. Standard IO multiplexes the peer's console output with the frames carrying the call over one byte stream and reserves sixteen byte values for the framing, every one of them a UTF-8 lead byte. The relay added for standard error covered one door into that stream; it could not cover erlang:display_string/1, or a NIF writing to a descriptor, and an accented character through either takes the control process down - refusing an install that was about to succeed. Nothing outside :peer can harden it, because the shared stream is the mechanism. A socket carries frames only, and a raw write goes to the null device the peer is detached onto. Measured rather than assumed: a peer on a socket needs no epmd entry, no cookie and no node name, reports nonode@nohost and is_alive() == false, and boots in around a quarter of a second. Erlang-level output still reaches the operator, because it travels the connection through the peer's user process - which is now what the standard-error relay is for, since a detached peer's descriptors are the null device. A peer whose origin dies still goes: the connection is its whole attachment, and the control process holding this end is linked. What it costs is the diagnosis of a peer that cannot boot. Detached, it says nothing on the way down and the origin holds no handle on it, so a failed boot is noticed when wait_boot expires rather than at once with the emulator's reason. That was weighed and taken: a boot script that will not boot is a broken release either way, unpack has verified the tarball it came from, and the preflight has established that the script and the emulator are there - while a provider writing a diagnostic is a release that works. The deadlines are options now, defaulted as before. A deadline nothing can shorten is a deadline no test can show is enforced, and there are two tests that give it a second and read the refusal. Refs: #13 Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
The notes described a peer on standard_io and four load-bearing properties. There are five now and one of them changed, so: the control connection is a socket and why, including what it costs and why that was the trade; the standard-error relay is what makes a detached peer's diagnostics arrive rather than what protects a shared stream; and the compile-environment check is described as the thing that stops a release Elixir will not boot reaching install_release/1, with an instruction not to weaken it into something that skips what it does not recognise. The claim that the deadlines were untested is gone, because they are not: they are options now and two tests read the refusal. The note on how the peer tests avoid passing for the wrong reason is new, since that is the property that will decay first as Elixir moves. The release note gains the check, which is user-visible: a version whose runtime configuration contradicts what it was compiled against is now refused where refusing is free. Refs: #13 Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
Materialisation seeded from sys.config and then wrote the result back over
it, so a second run resolved the providers over the first run's output.
Providers are not obliged to be idempotent and the familiar ones are not:
`if System.get_env("FEATURE"), do: config :app, feature: true` in a
runtime.exs sets a key on a run where the variable is set and says
nothing about it on a run where it is not. Install with it set, commit
without, and feature: true was still there - so the version an operator
made permanent was configured differently from the way it would boot,
and a value resolved once persisted after the provider had stopped
supplying it. Documenting that did not make the result equivalent to
Elixir's pipeline, which is what this issue claims, because a boot
resolves over the release's own configuration every time.
So the release keeps that configuration. sys.config cannot be it, being
the file release_handler reads and therefore the one the resolved result
has to land in, so the first materialisation of a version copies it to
sys.config.pristine and every one after that seeds from there. The copy
is made with an exclusive create, so that two installs racing cannot
make it twice and whichever loses reads what the winner captured rather
than a sys.config that may already have been resolved. It is made only
for a release that has providers: one without them is never rewritten
and has nothing to be protected from.
This is permanent design rather than a step in the migration. The
build.config path beside it has always had a pristine base - build.config
is one, and generate/1 only ever reads it - and that is the one thing the
old mechanism got right. When step 3 deletes that path this is what
carries the property forward. Not named build.config, because that name
is the discriminator and a file by it would send the release back down
the path being removed.
sys.config gains a CASTLE_MATERIALISED comment line, which makes the
invariant checkable: written by Castle, so a base must exist. A version
that says so with no base beside it has lost its original - materialised
by a build of this branch that kept none, or someone removed it - and is
refused, naming the remedy, rather than having a once-resolved
configuration captured as though it were pristine. Mix's coding pragma
stays the first line, where the emulator looks for it, and the line is
taken from the base, which never has one, so it cannot accumulate.
The header the base carries is Mix's, and is re-emitted because the
launcher reads it. The base needs no marker stripping: what Elixir writes
on its way to a reboot is stripped from the output, and with a base to
seed from it could not have accumulated there anyway.
An extra file in a version directory disturbs nothing. release_handler
names sys.config, start.boot, relup and <name>.rel; its only directory
listing is the recursive delete that removes a version, which removes
whatever is in it. Forecastle names every file it touches and globs
nothing. A tarball is built at assembly time on a build host, and this
file only comes into existence on a target during an install.
The scratch file is now copied from sys.config and then overwritten with
the base, because a copy carries the mode of its source and a write does
not: the file about to become sys.config keeps sys.config's permissions,
which matters for a file that holds resolved secrets.
Idempotence is asserted against a control rather than a fixture: two
versions of one release in a single root, sharing a runtime.exs so the
state the providers carry is identical, one materialised twice with the
environment changing in between and the other once with the environment
as it ended up, and the two configurations have to be equal term for
term. Against the previous commit it fails with feature: true still
present.
Refs: #13
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
Two findings in the base the previous commit added, sharing a remedy. An exclusive create makes creation atomic, not publication. The file exists and is empty between the open and the write, so a concurrent install told the name was taken could read a base that is not a configuration - and an install that died in that window left a truncated one that every later evaluation would prefer to the original still sitting in sys.config, permanently, with the CASTLE_MATERIALISED check unable to notice because sys.config had not been rewritten yet. And File.write/3 creates with the process umask, not with the mode of what it is copying. An operator restricting sys.config because its configuration or its serialised provider state holds credentials got a group- and world-readable copy of both beside it. That is the same class caught for the scratch file one commit ago - a copy carries the mode of its source, a write does not - applied at one site and missed at the other, which is worth naming rather than quietly correcting: the base is the more sensitive of the two, being permanent. So the copy is staged under a name of its own, given sys.config's mode, and published by hard link. A link publishes a file that is already complete, in a step that either happens or does not, and refuses rather than replaces if the name is taken - verified: :eexist against a file and against a directory, the destination untouched in both cases, and the mode carried through because a link is a second name for the one inode. Whichever install loses reads what the winner published, never its own staging copy. The staged file is emptied, given its mode, and only then filled, so what exists while the mode is still the umask's has nothing in it to read. Every failure path publishes nothing. A staging write or chmod that fails returns before the link; the link failing returns; the process dying before the link leaves an unpublished file under a name nothing reads, and the next evaluation resolves from sys.config, which is still the original, and publishes properly. Dying after the link leaves a second name for a base that is correct and complete. Staging is not tidied up on sight: an install cannot tell its own leftovers from another install's work in progress. A base that cannot be read as a configuration is now refused with the remedy named. It is preferred to sys.config by definition, so failing loudly is all that is left - and a release poisoned by the window this commit closes is exactly what that catches. stage/3 and publish/2 are public, alone among the file handling here, because the window between them is what makes a concurrent install safe and a window nothing can stand in is a window nothing can test. One test takes the two steps one at a time and looks at the destination in between, where it finds no file rather than a partial one, then checks that publishing a second, different staged file is refused. Three of the four new tests fail against the previous commit: that one for want of the functions, the mode one at 0644 against 0600, and the unreadable base for want of a remedy. Refs: #13 Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
File.cp/2 carries the source's mode, which is why the scratch copy was
switched to it two commits ago - but it writes the whole file first and
narrows it afterwards: :file.copy(src, {dest, [:exclusive]}) and then
copy_file_mode/2, at file.ex:1285. So a sys.config an operator had
chmodded to 0600 briefly existed as a 0644 castle-*.config, at a
predictable path, holding that configuration and its serialised provider
state. Right about the what, wrong about the when.
That is the third instance of one class across two rounds of review, and
each time it was fixed at the site where it was noticed. The remedy is
not a third careful call site: write_like/3 is now the only way this
module brings a file into existence, and the ordering lives inside it -
create empty, set the mode from the file it came from or is about to
become, then fill. There is no moment at which one of these files holds a
configuration and is wider than it should be, and the next site nobody
has written yet gets that for free. copy/2 is gone; File.cp/2 no longer
appears here.
Both files go through it: the pristine base, which used to build its own
sequence inline as stage/3, and the scratch copy the providers resolve
into, which used to be a copy plus an overwrite. The one remaining plain
write is the second write to the scratch, which by then exists with the
right mode and keeps it.
Audited every other file this creates. File.ln, File.rename, File.rm,
File.ls, File.stat, File.read and File.chmod do not bring a
configuration-bearing file into existence: the link names an inode that
already has the right mode, and the rename carries the scratch's mode onto
sys.config. Outside this module, Castle.Commands.write_sys_config/2 on
the build.config path creates sys.config with the process umask when it
does not exist yet. Same class, milder shape - no transient exposure,
since the mode it is granted is the mode it keeps - and left alone
deliberately: that path is deleted in step 3, and nothing observable may
change for a release assembled by today's Forecastle until then. Recorded
in AGENTS.md rather than silently skipped.
The test observes the mode before the content exists, which is the only
way to see this at all: create_like/2 is called on its own and the file is
found already restricted and still empty. A test of the end state cannot
distinguish the two orderings - both end at 0600 - which is precisely how
this survived a round that had already named the class, so the
in-peer observation of the scratch file's mode is recorded as a regression
guard rather than passed off as a discriminator. It passes against the
previous commit; the two primitive tests do not.
Refs: #13
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
A sys.config at 0440 - an operator declaring their configuration
read-only - made materialisation impossible. The mode went on while the
file was still empty, which was the point, but File.write/2 reopens the
path rather than writing through a handle held from creation, so filling
a file its own owner had just chmodded to 0440 failed :eacces and the
install stopped with a diagnostic about a temporary file. Verified both
halves: the write returns {:error, :eacces} at uid 501, and it succeeds
if the mode goes on afterwards.
It also showed up a tension in the last round's rule, which said to set
the mode before writing: you cannot, if the mode forbids writing. 0600
resolves both at once rather than trading between them. The file is
created owner-only, filled through that, and given the model's mode last.
Nothing is granted to group or other at any point, so the transient state
is narrower than the destination rather than merely different from it -
the property the last round wanted, now holding a fortiori instead of by
coincidence - and the file stays writable by its owner while there is
writing to do.
Which for the scratch is longer than one write: this module fills it, the
peer's pipeline writes the resolved configuration over it, and this
module writes it again. So its mode goes on after all three, immediately
before the rename, rather than at creation. Applying it any earlier would
have stopped the pipeline that is meant to fill the file. The two
operations that move one of these files into place are unaffected either
way - the link that publishes the base and the rename that replaces
sys.config need permission on the directory, not on the file - so a
restrictive mode never has to be relaxed again. Checked.
A failure part-way through now leaves the file narrower than intended
rather than wider, which is the right direction for the one that can
fail.
write_private/2 is the way a file comes into existence here; write_like/3
is that plus the mode, for a file written once. The comment says why it is
create-restrictive-then-widen and not the reverse, and says that the fill
reopens the path, since that is the fact that makes the reverse fatal and
the ordering look arbitrary without it.
Ownership is not reproduced, only the mode bits, and AGENTS.md now says
so as a property rather than an apology: chown needs privileges a release
account does not have, and where Castle could chown it is running as root,
which is worse than the problem. A deployment restricting sys.config by
group needs the release account's default group to suit the version
directory.
The test that pins this is a release whose sys.config is 0440,
materialised twice, with both the published base and the resolved output
ending at 0440. Against the previous commit it fails where the finding
said it would: permission denied, writing the staged base.
Refs: #13
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 814cb35923
ℹ️ 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".
| @doc false | ||
| @spec create_private(Path.t()) :: :ok | {:error, String.t()} | ||
| def create_private(path) do | ||
| with :ok <- write(path, ""), do: chmod(path, 0o600) |
There was a problem hiding this comment.
Create the temporary file at 0600 atomically
When the process has a typical permissive umask such as 0022, write(path, "") creates this file as 0644 before the following chmod; another local user who can traverse the release directory can open it during that window and retain the descriptor while write_private/2 subsequently fills it with configuration secrets, because the later chmod does not revoke an already-open descriptor. Use a creation primitive that applies 0600 as part of the initial exclusive open rather than narrowing the file afterward.
AGENTS.md reference: AGENTS.md:L74-L88
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and deferred rather than declined — with the repository owner's agreement, and recorded on the issue so it cannot be lost. It is the first work of step 2, landing before forecastle#6.
The detail that makes it a P1 rather than a transient blink is the one you named: chmod does not revoke an already-open descriptor, so a process that wins that window keeps reading everything written afterwards. Agreed on all of it.
Why it is being merged anyway: the affected path is dormant. Forecastle.post_assemble/1 calls rename_sys_config/1 as an unconditional tap, so every release Forecastle assembles today has build.config, and Castle.Commands.materialise/2 sends any release with build.config down the old generate/1 path. Nothing in existence reaches Castle.Peer. forecastle#6 — step 2 — is what changes that, which is exactly why the fix is sequenced immediately before it rather than after.
On the remedy: there is no open in OTP that takes a creation mode, so "apply 0600 as part of the initial exclusive open" is not directly expressible. The fix will instead create both files inside an owner-only working directory — mkdir, chmod 0700 while it is still empty, so a traversal window exposes nothing — fill them there, and rename out. File.rename and File.ln need permission on the directory rather than the file, which was measured during this branch, so a restrictive final mode does not have to be relaxed to publish.
Worth recording for context, since it bears on how the fix should be built: this is the fifth finding in this class on this branch, and each fix revealed the next — File.cp applies the mode after writing; File.write ignores modes; setting the model's mode before writing breaks a 0440 model outright; create-at-0600-then-widen fixed that but creation is still write-then-chmod. Every individual fix was locally reasonable. What kept failing was reasoning about one call site at a time, which is why the remedy is a directory that makes the whole sequence safe rather than a fifth careful call site.
Refs #13 — step 1 of three. Targets
release/1.0.0, the 1.0.0 integration branch.See the staged-landing comment
for why this lands in three parts. In short: doing #13 and forecastle#6 as one change
means a broken integration branch in between, because the peer needs a target that
still has its normal provider pipeline while forecastle#6 is what gives it one.
What this step does
Adds peer-based materialisation of the target release's configuration, taken when
the target has a normal
Config.Providerpipeline. The existingbuild.configpathstays as the fallback.
Nothing observable changes yet.
Forecastle.post_assemble/1callsrename_sys_config/1as an unconditionaltap, so every release Forecastle assemblestoday has
build.config, and the discriminator sends those down the old path. Norelease in existence reaches the new code. Forecastle's e2e suite passed on every
iteration of this branch, which is the check on that.
Why a peer at all
Castle.generate/1folds the stashed providers in the currently running release,even when the configuration it is producing belongs to a different version — and a
provider module can have changed between the two. Configuration for B has to be
evaluated by B's code.
So: a temporary BEAM on the target's own
prebootscript and its own ERTS, resolvedfrom the target's
.rel, with no distribution.Config.Provider.boot/1is thepipeline — Castle does not iterate providers anywhere. The only thing arranged is that
the pipeline writes instead of configuring a VM that is about to be discarded.
Details that took several passes
Compile-environment validation.
Config.Provider's write-and-reboot branch doesnot validate
validate_compile_env— validation lives in the other branch and in thesecond boot. Forcing the first and eliminating both meant a release could materialise
successfully and then be unable to boot, which on a restart transition surfaces as an
unexplained rollback. Elixir's own validator is now invoked, with a guard that
refuses an unrecognised shape rather than skipping, so a change in Elixir's
internals stops the install instead of waving it through.
A loopback socket, not shared stdio.
:peer'sstandard_ioframing reservessixteen byte values, all UTF-8 lead bytes — so a provider writing non-ASCII to stderr
corrupted the control channel and failed a materialisation that was about to succeed.
Reproduced, then fixed by moving the control connection to a loopback TCP socket. Still
no EPMD, no cookie, no node name; the peer reports
nonode@nohost. The stderr relaystayed: with TCP the peer's
userprocess is the connection, so it is the only reasona failed provider's diagnosis reaches the caller rather than the null device.
Every evaluation resolves from
sys.config.pristine. Materialising over theprevious output makes the result path-dependent: a conditional
runtime.exsentry setduring install and absent at commit would linger, where a real boot would drop it. So
Mix's original is preserved once and every evaluation seeds from it — which also makes
commit-time materialisation correct rather than harmful, since it then produces what a
boot at commit time would produce. The base is staged and published by hard link, so
publication is atomic and no-replace; an interrupted first attempt cannot leave a
truncated base that later runs would prefer forever.
One primitive writes these files. Four separate findings were the same class — mode
preservation — at four different call sites.
File.cpapplies the mode after writing;File.writeignores modes; setting the model's mode before writing breaks a read-onlymodel outright. They now go through one primitive that creates the file private, fills
it, and applies the model's mode last, so a partial failure leaves it narrower than
intended rather than wider. Ownership and group are not reproduced, only the mode bits;
that is documented as a property.
Known finding carried into step 2
One
[high]is outstanding, deliberately, andrecorded on the issue: private creation
is not atomic.
File.write(path, "")creates the inode with the process umask beforethe chmod, and
chmoddoes not revoke an already-open descriptor, so a racing localprocess could retain a readable one.
It is being merged because the affected path is dormant — nothing reaches
Castle.Peeruntil forecastle#6 — and it is the first work of step 2, before thatchange makes the path reachable. The fix is an owner-only working directory, since OTP
has no
openthat takes a creation mode.Tests: 39 → 77
Castle had 39. New coverage includes the discriminator for each shape, the
build.configpath behaving exactly as before, and — the case that justifies the wholeissue — a provider module compiled twice, one version loaded in the test node and the
other on the peer's code path, asserting the answer came from the peer's.
Also: idempotence checked against a control version materialised once in the same root,
by full term equality rather than a spot assertion; the publication window observed
step by step; a
0440sys.configmaterialising successfully; peer cleanup assertedfrom outside via the OS pid; and both deadlines exercised.
Every regression test was checked for bite. Where a test passes against the previous
commit as well, it is labelled a regression guard rather than presented as proof.
Review
Five adversarial rounds plus a narrow confirmation pass, and seven findings — four
[high]. Every[high]was a way the configuration Castle installs could differ fromwhat Elixir's own pipeline produces, which is exactly what this issue claims to
guarantee.