fix: name a remedy that works for an unreadable RELEASES - #25
Conversation
The refusal for a system running from a synthesised release record diagnosed both causes - releases/RELEASES missing, or unreadable - and then prescribed one remedy, a restart. That is enough only for the first. Forecastle's env.sh creates the file when it is absent, guarded on [ ! -f ... ], so a file that is present and cannot be read is stepped over on every start: the node comes back on a freshly synthesised record and the refusal repeats. An operator following the message would have restarted indefinitely. The remedy is now split along the diagnosis it already made. Absent, restart. Present and unreadable, make it readable or remove it before restarting, because the release will not replace a file that is there. Found by adversarial review of the Forecastle half, ausimian/forecastle#16. It is the same defect class as the ERTS guard's own messages on this branch - a refusal that states more than it has established, or points somewhere that cannot resolve it - and the third instance of it, so AGENTS.md records why the two branches must not be collapsed back into one. Co-Authored-By: Claude <noreply@anthropic.com>
The release note said the remedy for a synthesised release record is a restart, and that the refusal says exactly that. Both were true only for one of the two causes it had just named. The record is synthesised when releases/RELEASES was absent or unreadable, and the env.sh fragment creates the file guarded on [ ! -f ... ] - so a file that is present and cannot be read is stepped over on every start, and the system comes back on another synthesised record. Restarting on that advice never terminates. The note now splits the two. Castle's message does the same, in ausimian/castle#25, so an operator meeting the refusal is told which case they are in rather than having to read this. Co-Authored-By: Claude <noreply@anthropic.com>
|
@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: ef225b5356
ℹ️ 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".
| "but check the file first, because which of the two happened decides whether a " <> | ||
| "restart is enough. If releases/RELEASES is absent, restart: the release " <> | ||
| "creates it before it starts. If it is present and could not be read, make it " <> |
There was a problem hiding this comment.
When releases/RELEASES was absent at boot but is created and becomes readable before this refusal is shown, the added two-way remedy matches neither current condition: the file is no longer absent, and it is not present-and-unreadable. This is a supported state—the node retains its synthesised record even if the file appears later—so telling the operator to infer the boot-time cause by checking the file now can leave them without the applicable remedy, which is simply to restart with the readable file in place. Describe the required pre-restart state rather than branching only on the file's current existence.
AGENTS.md reference: AGENTS.md:L451-L453
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, and the fix is a simplification rather than another branch. Fixed in 5de0e9f.
The third state is real and I had missed it: RELEASES absent at boot, created and readable since. The node keeps the record it synthesised in init/1 regardless, so the refusal still fires — and an operator told to check whether the file is "absent" or "present and unreadable" finds it is neither, and is left without advice at the exact moment a plain restart is all they need.
The error underneath it is the one you name: I was branching on the boot-time cause when what an operator can act on is the state the file has to be in. So the message now asks for one condition instead of analysing two:
Before restarting, make sure
releases/RELEASESis either absent or readable: the release creates that file only when it is absent, so an unreadable one left in place is stepped over on every start and the system comes back on the same synthesised record. Absent, or readable, is what a restart needs.
That covers all three states, and it is shorter than the branch it replaces — which is a fair sign the branch was the wrong shape rather than merely incomplete.
AGENTS.md records the dead end explicitly, since "branch the advice on why the record was synthesised" is the obvious correction and is what I reached for first. It now says not to turn it back into a case analysis of the cause, and not to collapse it into a bare "restart the system" either.
The test comment was carrying the same misconception — it described "the other branch of the remedy" — and now states why neither the bare restart nor the case analysis is right.
Worth noting this is the third refusal on this branch series to have claimed more than it could deliver, after the ERTS guard's message was wrong twice the same way. The pattern is consistent enough to be worth naming: a refusal that reports a diagnosis tends to overreach, and one that reports the condition required to proceed does not.
The two-way remedy branched on whether the file is absent or present and unreadable, which is a case analysis of why the record was synthesised. It has no advice for a third state: absent at boot and created, readably, since. The node keeps its synthesised record either way, so the refusal still fires, and an operator checking the file finds it neither absent nor unreadable - when a plain restart is exactly what they need. Naming the required condition instead covers all three and is shorter than the branch it replaces: releases/RELEASES absent or readable, then restart. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5de0e9ff1f
ℹ️ 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".
| "change the record this node works from. Restart the system: the release " <> | ||
| "creates the file before it starts."} | ||
| "change the record this node works from, so the system has to be restarted. " <> | ||
| "Before restarting, make sure releases/RELEASES is either absent or readable: " <> |
There was a problem hiding this comment.
When RELDIR or {sasl, releases_dir} relocates the release records, this instruction directs the operator to repair the root-relative releases/RELEASES, but :release_handler reads a different file. Restarting after following this remedy therefore leaves the node on another synthesised record; refer to the handler's configured RELEASES file, or explicitly scope the advice to default Mix deployments.
AGENTS.md reference: AGENTS.md:L279-L290
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and worse than the finding says. Fixed in fa8cc3d.
You are right that the remedy pointed at the root-relative file while :release_handler reads the one RELDIR or {sasl, releases_dir} names — I had just corrected exactly that conflation in the documentation two commits earlier and then reproduced it in the message, which is the same mistake a third time in the same series.
The part that makes it more than a wrong path: under those overrides, "absent" does not get the operator out either. The env.sh hook creates $RELEASE_ROOT/releases/RELEASES, so on a deployment that has relocated the records, leaving the handler's file absent means the release creates one the handler will not read, init/1 synthesises again, and the restart loops. The remedy there is not "repair a different path" but "put the file the handler reads in place yourself, because nothing else will". The message now says that rather than scoping the advice to default Mix deployments — scoping it would have left the affected operator with no remedy at all, which is the failure mode of the previous round's fix.
So the wording is now:
Before restarting, make sure the RELEASES file
:release_handlerreads is either readable or absent, because the release creates that file only when it is absent … That file isreleases/RELEASESunder the release root unlessRELDIRor thesaslreleases_dirparameter points elsewhere; where one of those does, the release creates a file at the root that the handler will not read, so the one it does read has to be put there and made readable by hand.
AGENTS.md notes that this paragraph and that sentence both come out when #23 lands, since what they describe is precisely the divergence #23 removes — Castle following those overrides when it derives its own releases directory. Better to have the message tell the truth about the current behaviour than to describe behaviour Castle does not have yet.
mix precommit green, 112 tests.
The remedy said releases/RELEASES. That is the file the release creates, which is not necessarily the file release_handler reads: RELDIR and the sasl releases_dir parameter relocate the records, and Castle does not follow them yet (#23). An operator on such a deployment would have repaired a file the handler never opens and restarted onto another synthesised record. Under those overrides the remedy is genuinely harder rather than differently spelled, and the message now says so: the hook creates a file at the root that the handler will not read, so absent does not get them out either, and the file it does read has to be put there by hand. AGENTS.md records that this paragraph and that sentence both come out when #23 lands, since what they describe is the divergence #23 removes. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa8cc3d159
ℹ️ 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".
| remedy that works. It said to restart, and a restart alone is enough only when | ||
| `releases/RELEASES` is absent or readable: the release creates that file when it | ||
| is missing, so a file that is present but unreadable is stepped over on every |
There was a problem hiding this comment.
When RELDIR or {sasl, releases_dir} relocates the handler's records, this release note still says that an absent root-relative releases/RELEASES makes a restart sufficient; the release hook creates that root file, but the handler continues reading the relocated path and synthesises another record. Although the command text now handles this override, the user-facing release note preserves the same incorrect remedy and should identify the configured handler file or scope the claim to default Mix deployments.
AGENTS.md reference: AGENTS.md:L460-L468
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 615b69d. You are right, and it is the same correction I had just made to the command text and to AGENTS.md while leaving the note an operator actually reads still saying the wrong thing — the third time in this series that the fix landed in one place and not its restatement, which is the argument for the Castle.Deployment.root_dir/0 single-home convention rather than against it.
The note now identifies the file instead of assuming it: releases/RELEASES under the release root unless RELDIR or the sasl releases_dir parameter points elsewhere, and where one of those does, that a restart cannot fix it alone because the release creates the root file the handler will not read. It points at #23 for Castle following the overrides itself.
I also swept both documents for other unqualified uses. The remaining mentions describe mechanism — what the handler reads at startup, what an unpack writes back — rather than prescribing a remedy, and the make_releases/0 entry was already qualified in the commit before this one.
mix precommit green, 112 tests.
There was a problem hiding this comment.
Fixed in fdfa7f5. Correct — init/1 reads it with file:consult/1, so malformed terms fail exactly as permissions do, and the hook leaves an existing file alone whatever is in it. "Readable" would have sent an operator with a corrupt RELEASES round the same loop the message exists to end.
The condition is now absent, or consultable, and the message says why permissions are not the whole of it. Sweeping for the same wording turned up the record-check entry higher in the release note, which still described the cause as the file "not being there" and the remedy as a bare restart; both corrected.
This is the fourth narrowing of this one sentence, and each has been a real defect rather than a rewording, so worth naming the pattern: every version stated a condition that was sufficient for the case I had in mind and not necessary in general — restart, then absent-or-present, then readable. "Consultable by :release_handler" is the first one phrased as what the handler actually requires rather than as what I expected to have gone wrong.
mix precommit green, 112 tests.
There was a problem hiding this comment.
Right on the mechanism again — init/1 accepts only {ok, [Term]}, so two valid terms consult fine and are still rejected. Fixed in 8a83d12, and fixed differently from the previous four, because the series itself was the signal.
Each version named a property of the file, each was necessary and not sufficient, and each admitted a narrower counterexample: restart → present-but-unreadable; absent-or-present → created readably after boot; readable → malformed terms; consultable → two valid terms. There was no reason to expect that to stop, and the underlying error was mine rather than incremental: I kept stating the condition I expected to have gone wrong instead of the condition the handler imposes.
So the message now asks for a file :release_handler accepts, and says explicitly that present, readable and parsing are each necessary and none sufficient. That cannot be narrowed again, because it claims no mechanism — and it is the more useful thing to tell an operator anyway, since the handler is what has to take the file.
AGENTS.md records the whole series and says not to substitute whichever internal criterion is current, that being the move that was wrong five times.
mix precommit green, 112 tests.
The command text was corrected to name the file release_handler reads rather than the root-relative one, and the release note was left saying that an absent releases/RELEASES makes a restart sufficient. Under RELDIR or the sasl releases_dir parameter it does not: the hook creates the root file, the handler goes on reading the relocated one and synthesises another record. Same correction, same reasons, now in the note an operator actually reads. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 615b69df6f
ℹ️ 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".
| "creates the file before it starts."} | ||
| "change the record this node works from, so the system has to be restarted. " <> | ||
| "Before restarting, make sure the RELEASES file :release_handler reads is " <> | ||
| "either readable or absent, because the release creates that file only when it " <> |
There was a problem hiding this comment.
When this file is permission-readable but contains malformed Erlang terms, release_handler:init/1 still falls back to the synthesised record because file:consult/1 fails, while the preboot hook leaves the existing file untouched. An operator following the new “readable” condition can therefore restart indefinitely; describe the required state as absent or valid/consultable by :release_handler, and align the release note with that wording.
AGENTS.md reference: AGENTS.md:L440-L446
Useful? React with 👍 / 👎.
release_handler:init/1 reads the file with file:consult/1, so a file whose permissions are fine but whose terms are malformed fails in exactly the same way as an unreadable one - and the hook leaves an existing file alone whatever is in it. The message said readable, which sent an operator with a corrupt RELEASES round the same restart loop the message exists to end. Absent, or consultable, is the condition. Also corrects the record-check entry higher in the release note, which still described the remedy as a bare restart and the cause as the file not being there. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdfa7f5926
ℹ️ 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".
| "creates the file before it starts."} | ||
| "change the record this node works from, so the system has to be restarted. " <> | ||
| "Before restarting, make sure the RELEASES file :release_handler reads is " <> | ||
| "either absent or one it can consult - it reads that file with file:consult/1, " <> |
There was a problem hiding this comment.
When the existing file contains two syntactically valid Erlang terms, file:consult/1 succeeds, but release_handler:init/1 accepts only {ok, [Term]} and otherwise synthesises the empty record again. Thus an operator can satisfy this new “consultable” criterion, restart, and receive the same refusal indefinitely; require a structurally valid single-term RELEASES file accepted by :release_handler, rather than merely one accepted by file:consult/1. Fresh evidence in the revised text is its explicit equation of the remedy with file:consult/1 success.
AGENTS.md reference: AGENTS.md:L460-L466
Useful? React with 👍 / 👎.
Fifth correction to one sentence, and the last, because it stops naming a
property of the file. Each earlier version gave a condition that was
necessary and not sufficient, so each admitted a narrower counterexample:
restart missed a present-but-unreadable file; absent-or-present missed one
created readably after boot; readable missed malformed terms, since init/1
reads it with file:consult/1; consultable missed a file of two valid terms,
since init/1 accepts only {ok, [Term]}. The hook leaves an existing file
alone whatever is in it, so every one of those states loops.
The remedy now asks for a file release_handler accepts, and says that no
single property of the file is the test. It cannot be narrowed again because
it claims no mechanism, and it is what an operator needs regardless: the
handler is the thing that has to take the file.
AGENTS.md records the series, so the next reader does not substitute
whichever internal criterion is current - the move that was wrong five
times.
Co-Authored-By: Claude <noreply@anthropic.com>
|
@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". |
1 participant
Refs #13. Targets
release/1.0.0. Found by adversarial review of the Forecastle half, ausimian/forecastle#16.The defect
The refusal for a system running from a synthesised release record diagnoses both causes and then prescribes one remedy:
The restart is enough only for the first. Forecastle's
env.shcreates the file guarded on its absence —[ ! -f "$RELEASE_ROOT/releases/RELEASES" ]— so a file that is present and unreadable is stepped over on every start.release_handler:init/1fails to read it again, synthesises another empty record, and the refusal repeats. An operator following the message restarts indefinitely.The fix
The remedy is split along the diagnosis the message already made: absent, restart; present and unreadable, make it readable or remove it first, because the release will not replace a file that is there.
Message-only — the guard's behaviour is unchanged and correct. Refusing is right in both cases; only the way out differed.
Why it is worth a commit rather than a note
This is the third instance on this branch series of the same defect class: a refusal that states more than it established, or points at a remedy that cannot resolve it. The ERTS guard's message was wrong twice the same way. So
AGENTS.mdnow records why the two branches must not be collapsed back into one — the diagnosis admits two causes, so a single remedy is wrong for one of them.Verification
mix precommitgreen — 112 tests. Three assertions updated; one now also asserts the unreadable branch, which no test covered before.