feat!: remove the build.config path and gate upgrades on the record - #21
Merged
Conversation
Castle materialises a target's configuration in a peer running the target's own code, and from Forecastle 1.0.0 no release it assembles has a build.config, so the branch that read one was unreachable. Deleting it takes generate/1, the boundary function that called it, and the provider fold and sys.config writer it needed: the last copy Castle held of a pipeline Config.Provider already implements. What materialise/2 still has to answer is a path with no release at it. The peer's refusals name a file the version is missing, which is the right answer for a release that was unpacked and then damaged and the wrong one for a version that was never unpacked - and absent, empty and not-a-directory are all that second case, so all three say to unpack the release first. make_releases/0 no longer resolves the releases directory against the working directory. release_handler resolves its own relative paths against code:root_dir() - consult/2 is file:consult(root_dir_relative_path(File)), and do_write_release/3 the same - so the working directory was only ever visible to this function's own File.exists? guard, which is what let the file it looked for and the file OTP writes be different ones. It stays create_RELEASES/3, never /4 with the root supplied: the three-argument form passes an empty Root, and that is what makes check_rel_data/4 store library directories as lib/<app>-<vsn>, "to make it easy to create a relocatable RELEASES file". A root would bake this machine's paths into a file whose point is that it can be moved, where no assertion on the end state would find them. upgradable/0 is where bin/castle's shell test for releases/RELEASES belongs, because the file is not the question. release_handler reads it once, in init/1, and otherwise synthesises a record from the boot script's name and version with the libs field left empty; nothing later replaces that record, and an upgrade from it reports success while leaving any application whose version changed, but whose code the relup does not load, running from the library directory of the release being replaced. The empty application list is the exact discriminator - which_releases/0 reports mk_lib_name(Libs), mk_lib_name([]) is [], and a record read from a file names at least kernel and stdlib - and asking the node is what sees the case the shell cannot: a file that appeared after the boot that went looking for it. BREAKING CHANGE: Castle.generate/1 is removed, along with the build.config path through install/1 and commit/1. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
AGENTS.md described the two-shape dispatch as a load-bearing property and recorded the umask gap in write_sys_config as a known one; both belonged to the path that has gone, and a note about a gap in deleted code reads as a gap in the code that is left. The boot-time race on sys.config went with it too - nothing Castle does runs at boot any more - and the cold-boot interaction it listed as unreachable is reachable now that forecastle#6 has landed, so what is missing there is a test rather than a dependency. RELEASE.md is notes for an unreleased 1.0.0, so generate/1 is not a function this release changes but one it removes: 0.3.1 published it. The gate and the make_releases change are operator-visible in their own right - one refuses an upgrade that used to be accepted, the other stops a caller having to change directory first. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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
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 #13 — step 3 of three, the last of the keystone. Targets
release/1.0.0.Steps 1 and 2 are merged: Castle materialises the target's configuration in a
:peerrunning the target's own code, and Forecastle no longer intercepts configuration at all,
so no release it assembles has
build.configany more. This is the cleanup that promised,plus two changes Forecastle's work specified along the way.
Removed: the
build.configpathCastle.Commands.generate/1,expand/1,write_sys_config/2, thebuild.configbranchof
materialise/2, and the publicCastle.generate/1. Dead with them: the configprovider stub and its tests.
Nothing called it — Forecastle's only remaining reference is a negative assertion,
refute env_sh =~ "Castle.generate", a test that the call is gone.materialise/2is now one path. The surviving case is "there is no release here", and ituses
File.lsrather thanFile.dir?deliberately: the peer's own refusals name amissing file, which is right for a release that was unpacked and then damaged and wrong
for one never unpacked — and an empty directory belongs with the absent one.
Changed:
make_releasesno longer depends on the working directoryrelease_handlerresolves relative paths against:code.root_dir(), not the cwd —consult/2anddo_write_release/3both go throughroot_dir_relative_path/1. So theonly cwd-sensitive thing here was ever its own
File.exists?guard, and callers no longerneed to change directory to use it. That matters because the working directory is
VM-global: doing this over
rpcon a live node meant any application performing relativefile I/O during the call resolved under the release root.
It must stay
create_RELEASES/3, never/4with the root supplied./3iscreate_RELEASES("", RelDir, RelFile, LibDirs), andcheck_rel_data/4branches on anempty
Rootto store lib dirs relative — OTP's comment says this is "to make it easy tocreate a relocatable RELEASES file". Verified by experiment rather than argument:
/3with an absolute directory wrote
lib/kernel-10.5;/4with the root wrote/var/folders/…/lib/kernel-10.5. Passing the root would bake absolute lib paths in andsilently make the release non-relocatable, which no end-state test would catch. The code
comment and a unit test both say so.
The directory is an argument rather than self-derived, matching this repo's existing
convention — under
mix test,:code.root_dir()is the OTP installation, which has areleases/RELEASES, so a self-derived directory short-circuits every test to{:ok, []}and makes the error paths untestable.
Added:
Castle.upgradable/0A system whose
releases/RELEASESwas missing when it booted runs from a record OTPsynthesised from the boot script — which names no applications. Upgrading from that
state reports success and leaves any application whose version changed, but whose code
the relup does not explicitly load, running its old code.
Confirmed in
sasl-4.3:init/1falls back to a#release{}whoselibsis[];which_releasesreportsmk_lib_name(Libs)andmk_lib_name([]) -> []. And the reasonit is silent rather than loud:
get_new_libs([], _) -> [], andpoint_of_no_returncallscode:replace_path/2only overnewlibs.So the discriminator is exact rather than heuristic — a synthesised record reports an
empty application list, and a real one cannot, because
kernelandstdlibare alwayspresent.
upgradable/0refuses in that state and names the remedy. Note what it must say and why:the file cannot be repaired from a running node, because
release_handlerreads it once,in
init/1. Creating it now changes nothing about the record this node works from. Thesystem has to be restarted — and the release creates the file before it starts, so a
restart is all that is needed.
This replaces a shell-side approximation in
bin/castle, which tested whether the fileexisted. That passes in exactly the case the gate exists to catch: a file that appeared
after the boot which looked for it.
Docs corrected rather than appended to
AGENTS.md: the two-shape dispatch is no longer described as load-bearing (it recordsthat the second path existed and must not come back); the
write_sys_configumask notewent with the code it described; the concurrent-boot
sys.configrace is deleted,because nothing of Castle's runs at boot now; the cold-boot limitation is re-scoped from
"unreachable" to "reachable, untested, and Forecastle's to test"; and a stale claim that
the pinned Forecastle "never calls this" is gone — it does.
RELEASE.md:generate/1removed from the entries that described it, aRemovedentryfor the path, a
Changedentry for the cwd independence, and anAddedentry for thegate. README left alone; its staleness is tracked under #9.
Verification
mix precommitgreen — 86 tests. Compiled and tested against Elixir 1.20.0/OTP 28and 1.18.3/OTP 27 as well: clean, 86 passing on both.
Cross-repo, which is the only thing that exercises the peer path end to end: Forecastle's
suite with its fixture pointed at this branch,
--include e2e, 170 tests, 0 failures.The installed version carries
CASTLE_MATERIALISED=trueand asys.config.pristine, withMix's own header preserved.