fix: accept the two release files an unpacked release has - #20
Merged
Conversation
Castle.Peer.release_file/1 listed the version directory, filtered for *.rel and required exactly one, refusing anything else as ambiguous. An unpacked release always holds two, so the peer path refused every release it will ever be asked to configure - found by Forecastle's e2e suite the first time it drove this path. Mix writes one release file into the version directory, <name>.rel, the one belonging to the start script. release_handler adds a second on unpack: it extracts the tarball's <name>-<vsn>.rel, reads that to decide what the version is - check_rel on the way in, and RELEASES written from the record it returns - and then copies it in beside Mix's, "keeping this for backwards compatibility reasons with older systools:make_tar, where there is no copy of the .rel file in the releases/<vsn> dir. See OTP-9746." For a tarball systools built the two names are the same and that copy overwrites Mix's, which is why OTP's comment reads as harmless. They differ here, so both survive. Two shapes are therefore expected and no others: one file, which is a version as Mix assembled it, and that pair, which is an unpacked one. release_handler's copy is the one read, because it is the file the version was admitted on and the applications and emulator version RELEASES records came from those very bytes - so if the two ever disagreed, it is the release as the system understands it. They are byte-identical in practice: Forecastle writes the tarball's copy from Mix's with File.cp!, the tar carries it unchanged, and release_handler's copy is a read and a write of the whole file, all three checked rather than assumed. Nothing compares them, deliberately. This is a choice of which file is authoritative rather than a tie-break between equals, so it holds however the two differ, and a difference in bytes that cannot change the answer is no reason to refuse an install that works. The pair is recognised by the version alone, which is the version directory's own name - release_handler copies into releases/<Vsn> for the Vsn it has just read out of the file, and Mix assembles into releases/<version> - so the release name never has to be found from anywhere. Anything else refuses as before and still names what it found: two names that are not that pair are two release files rather than two copies of one, and so is any third. The comment saying a version directory holds exactly one is what shipped this. It was true of an assembled release and false of every unpacked one, which is the only kind this path meets, and Castle.SyntheticRelease modelled the assembled shape faithfully enough that seven rounds of review had nothing to notice. So the fixture now builds the unpacked shape as well - and builds it by default, since the peer path is reached from install and commit and so meets a version that came out of a tarball. 29 of the 45 tests in peer_test.exs fail against 3ec5979 for that reason alone, every one that materialises anything, which is the gap that mattered here rather than the missing clause. Six tests are new. The unpacked shape resolves, and its two files are asserted to hold the same bytes. The copy unpacking left is the one read, shown by planting a bogus emulator version in Mix's copy: with the two identical, nothing but a difference between them can show which was read. A version Mix assembled still resolves from its one file. And three directories that are genuinely ambiguous are still refused - two unrelated names, a copy belonging to another version, and a third file beside the pair - the second of which is what stops the pair being recognised by suffix rather than by this version. No RELEASE.md entry. The peer path is new in this unreleased version and has never been reachable from a released one, so no consumer could have seen the refusal; what the Added entry already promises is what now works. Verified on 1.19.5 and on 1.20.0 through mise, a recent change here having compiled clean on 1.19 and failed 1.20's type checker: mix compile --warnings-as-errors is clean and the suite passes 90 under both. Refs: #13 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. What shall we delve into next? 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 #13. Targets
release/1.0.0.Found by Forecastle's end-to-end suite the first time anything drove the peer path,
while landing forecastle#6. It blocks that PR:
bin/castle installfails at the firststep, so 21 e2e tests cannot run.
The bug
Castle.Peer.release_file/1lists the target's version directory, filters for*.rel,and requires exactly one. An unpacked release always has two.
release_handler:do_unpack_release/4(sasl-4.3:1549-1553) copies the tarball's.relintoreleases/<vsn>/, commented "Keeping this for backwards compatibilityreasons with older systools:make_tar, where there is no copy of the .rel file in the
releases/ dir. See OTP-9746." The tarball entry is
<name>-<vsn>.rel.<release>.rel(
mix/tasks/release.ex:1305).Different names, so both survive. With a tarball
systoolsbuilt they collide and thecopy overwrites — which is why OTP's comment reads as harmless.
The old comment is why this shipped: "A version directory holds exactly one: Mix renames
the one belonging to the
startscript and removes the rest." True of a freshlyassembled release; false of every unpacked one — and unpacked is the only kind
the peer path ever sees.
The fix
The pair is recognised by construction rather than by a suffix test: one name is the
other with
-<vsn>inserted, checked symmetrically. Only the version is needed, and itis already to hand as the version directory's basename — the release name never has to be
known, which avoids a dependency on
RELEASE_NAMEor onRELEASESthat the functioncould not verify.
<name>-<vsn>.relis the one read, because it is the file the version was admitted on:do_unpack_release/4validates it and writes theRELEASESentry from the record itreturns, then copies those bytes into the version directory. The two are byte-identical
in the normal case — verified through
copy_relfile/1,:erl_tar, andrelease_handler's owncopy_file/3— but the fix deliberately does not comparethem: preferring one is a choice of authority, not a tie-break, and only
{:erts, _}is ever read from it, so a byte comparison could only turn a difference that cannot
change the answer into a refused install that would have worked.
Accepted: one file; or two that are
<stem>.reland<stem>-<vsn>.relfor thisdirectory's version. Refused, with the message still naming what it found: zero; two that
are not that pair — unrelated stems, or a copy belonging to a different version — and
three or more.
The deeper defect was the fixture
Castle.SyntheticReleasebuilt the assembled shape, so the peer tests ran against alayout the production path never meets. It now builds either shape and defaults to
:unpacked, since the peer path is reached only frominstallandcommitand soalways meets a version that came out of a tarball.
That flip is the substantive part of this change. Against the previous commit, 29 of 45
peer tests fail once the fixture presents what reality presents — including one that
never reached the emulator check at all. Seven rounds of adversarial review on the peer
work could not find this, because every round checked the code against a fixture that
quietly encoded the wrong assumption.
Six new tests: the unpacked shape resolves; the handler's copy is the one read (a bogus
emulator version planted in Mix's copy — the only way to discriminate, since the two are
otherwise identical); the assembled shape still resolves; and three refusals.
No
RELEASE.mdentryThe peer path is new in this unreleased 1.0.0 and has never been reachable from a
released version, so no consumer could have met the refusal. A "Fixed" line would
describe a defect in a feature the same notes introduce.
Verification
mix precommitgreen — 90 tests. Also compiled and tested against Elixir 1.20.0/OTP 28in a separate build root: clean, no type-checker warnings, 90 passing.