Skip to content

Commit c0f7c07

Browse files
ausimianclaude
andcommitted
docs: correct seven claims the new @docs got wrong
Review of 01ccbf2 found seven inaccuracies, all of them assertions nothing in this project checks. Two were regressions of rules this repo had already settled rather than ordinary staleness, and both are now written down in AGENTS.md so the next draft does not repeat them. The two regressions: * `upgradable/0` said the record is synthesised when the system started without a `releases/RELEASES` file "it could read". That is the *readable* phrasing five successive corrections to the refusal message had already discarded - it misses malformed terms - and it named the file unqualified, which castle#23 forbids because RELDIR and `{sasl, releases_dir}` relocate it. The @doc now says what the refusal says: the file `:release_handler` accepts, no one property of the file being the test, and the path qualified. * `commit/1` said committing an already-permanent version "succeeds and changes nothing". That was true before castle#14 moved materialisation inside `commit`'s serialised region. `make_permanent/1` is still a no-op for such a version, but the target's `sys.config` has been rewritten from current provider inputs by then, and a provider that fails now fails the command. The five others: * `report!/1` does not make every failure a `Castle.Error`. `installed/5` settles the marker and re-raises what `install_release/1` raised, threw or exited with, so automation rescuing only `Castle.Error` would read a blown-up command as a success. The moduledoc now says which failures become which. * The install preflight is documented in the order it happens: the ERTS guard, the release-record lookup and its refusal, the pending-marker refusal, materialisation, arming - five steps, before `install_release/1` rather than before any `:release_handler` call, since the lookup is one. The pending-marker refusal was missing entirely, and it is what stops a refused caller replacing the configuration a pending install is about to boot. * "Anything that takes the system down brings the previous version back" is false for a restart install. The launcher consumes the marker pair on the next start and boots the target; it is the restarts after that one which return to the previous permanent version. RELEASE.md said the same thing and is corrected with it. * `bin/castle commit` with no version selects a `:current` release and exits non-zero when there is none - not "the version running now", which a system on its permanent release also has. * `unpacked` is staged and not currently installed, a version a rollback returned to that state included. `running/1`'s own @doc already said so. And the specs: two @specs on a five-arity defaulted definition left `install/2..4` unspecced, contradicting this branch's own claim that the whole public surface carries one. All five are now present, checked with `Code.Typespec.fetch_specs(Castle)` against `Castle.__info__(:functions)` rather than by reading the source - ExDoc renders only the widest arity's spec, so the page could not have shown the gap. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
1 parent 01ccbf2 commit c0f7c07

3 files changed

Lines changed: 240 additions & 73 deletions

File tree

AGENTS.md

Lines changed: 65 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -914,13 +914,23 @@ Castle's job is configuration and release management on a running node.
914914
reboot takes and which automation reads.
915915

916916
Every one of them is a command entry point, so `Castle` is the command
917-
boundary: an operation that fails raises `Castle.Error` there, which is what
918-
leaves a non-zero exit status behind for the shell that asked for it. Raising,
919-
not halting — the expression runs on the *running* node, so halting would take
920-
down the system under management; `Kernel.CLI` catches on the node and
921-
re-raises in the calling VM, and only that VM exits. `Castle.Commands` holds
922-
the operations themselves, returning their outcome instead of acting on the
923-
process, which is what makes them testable.
917+
boundary: an operation that fails raises there, which is what leaves a non-zero
918+
exit status behind for the shell that asked for it. Raising, not halting — the
919+
expression runs on the *running* node, so halting would take down the system
920+
under management; `Kernel.CLI` catches on the node and re-raises in the calling
921+
VM, and only that VM exits. `Castle.Commands` holds the operations themselves,
922+
returning their outcome instead of acting on the process, which is what makes
923+
them testable.
924+
925+
**`Castle.Error` is what `report!/1` raises, and it is not everything a command
926+
raises.** `report!/1` turns a returned `{:error, message}` into one; an
927+
exception, a throw or an exit that the operation did not handle goes straight
928+
past it. That is deliberate for the one place it can happen on purpose —
929+
`installed/5` settles the marker and re-raises `install_release/1`'s failure
930+
unchanged, folding it into a message only where the marker could not be settled
931+
— so anything claiming that a failed command raises `Castle.Error`, in a `@doc`
932+
or here, has to say which failures. Automation told to rescue `Castle.Error`
933+
and nothing else would treat a `release_handler` that blew up as a success.
924934

925935
`Castle.customize/1` is the one function in that module which is *not* one of
926936
them — it runs at build time, in a consumer's `mix.exs`, and returns a value
@@ -956,6 +966,54 @@ returns; a spec naming the lines, or an error tuple, would be describing
956966
kept by hand, and a claim in a `@doc` about what a command refuses is worth
957967
checking against `Castle.Commands` before it is trusted.
958968

969+
**A definition with defaults needs one `@spec` per arity, and "every public
970+
function carries a spec" is a claim about arities.** `install/1..5` is five
971+
functions and therefore five specs; the first version of this carried two, for
972+
`install/1` and `install/5`, and left three unspecced while this file and
973+
`RELEASE.md` both said the surface was complete. Nothing catches that, and
974+
`mix docs` is no help either: ExDoc renders one spec against a defaulted
975+
definition, the widest arity's, so the page looks the same with two specs as
976+
with five. `credo --strict` passes, and there is no Dialyzer. So check it with
977+
`Code.Typespec.fetch_specs(Castle)` and count — against
978+
`Castle.__info__(:functions)`, which is the list that has to be covered —
979+
rather than by reading the source, which is what missed three of them.
980+
981+
**The `@doc`s are claims, nothing checks them, and the first draft of them
982+
walked back into two rules this file had already settled.** Both are worth
983+
naming, because both were regressions rather than staleness:
984+
985+
* `upgradable/0`'s said the record is synthesised when the system started
986+
"without a `releases/RELEASES` file it could read" — which is the
987+
*readable* phrasing corrected above, admitting the malformed-terms
988+
counterexample, and it named the file unqualified besides. A `@doc` that
989+
describes a refusal has to say what the refusal says: name the authority
990+
(the file `:release_handler` accepts, no single property of it being the
991+
test), and qualify the path with `RELDIR` and `{sasl, releases_dir}`.
992+
* `commit/1`'s said that committing an already-permanent version "succeeds
993+
and changes nothing", which was true of the shape castle#14 replaced.
994+
`Commands.commit/5` materialises inside its own serialised region, so the
995+
target's `sys.config` is rewritten from current provider inputs before
996+
`make_permanent/1` — which is itself a no-op for that version — is called
997+
at all. OTP's step being idempotent is not the command being idempotent.
998+
999+
Three more were ordinary staleness of the same kind, and the pattern is that a
1000+
`@doc` describing a *sequence* goes stale where a `@doc` describing a value
1001+
does not: `install/1`'s claimed two steps before `:release_handler` was asked
1002+
for anything, when `install_upgradable/5` asks `which_releases/0` first and
1003+
refuses for a pending marker in between, and the line that matters is
1004+
`install_release/1` rather than any handler call; the rollback an install
1005+
leaves was described as "anything that takes the system down brings that one
1006+
back", which is false for the one reboot a restart install has already asked
1007+
for and the launcher is holding a marker to carry out; and `unpacked` was
1008+
glossed as "staged and never installed", which `running/1`'s own `@doc`
1009+
already contradicts. `bin/castle commit`'s argumentless form was described as
1010+
defaulting to "the version running now", where `castle.sh.eex` selects a
1011+
`:current` release and exits non-zero when there is none.
1012+
1013+
So: read a `@doc` about a sequence against the function that implements the
1014+
sequence, not against the module's summary of it, and read one about
1015+
`bin/castle` against Forecastle's `priv/castle.sh.eex`.
1016+
9591017
Forecastle is what arranges for these to be reachable: it leaves the
9601018
configuration Mix wrote alone, adds a `:preboot` script that starts `:castle`,
9611019
and writes the `env.sh` fragment and `bin/castle` wrapper that call into this

RELEASE.md

Lines changed: 31 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -254,7 +254,10 @@
254254
The rollback that provisional state buys is real and needs nothing:
255255
`make_permanent/1` is the only thing that writes `releases/start_erl.data`, so
256256
a provisional release that dies before `Castle.commit/1` is followed by an
257-
ordinary start of the version that was permanent before.
257+
ordinary start of the version that was permanent before. The one restart that
258+
is not that is the one the install asked for: while the pair is still
259+
unconsumed the next start boots the target, which is the reboot being carried
260+
out rather than a rollback being missed.
258261

259262
The two-stage `restart_new_emulator` transition remains unsupported. It reboots
260263
into a temporary hybrid release whose version directory holds a boot script and
@@ -267,25 +270,37 @@
267270
the only one meant to be called from other Elixir code, and everything else is
268271
a command entry point that `bin/castle` reaches over `rpc`. That distinction is
269272
worth reading before calling any of them, because a command prints its report
270-
and returns a bare `:ok`, and a command that fails raises `Castle.Error`
271-
instead of returning an error - which is what leaves a non-zero exit status
272-
behind for the shell, and is not what a caller expecting an ordinary function
273-
would write code for.
273+
and returns a bare `:ok`, and a command that fails raises rather than returning
274+
an error - which is what leaves a non-zero exit status behind for the shell,
275+
and is not what a caller expecting an ordinary function would write code for.
276+
The moduledoc is careful about what it raises, because automation acts on it: a
277+
refusal the command made and an error `:release_handler` returned become
278+
`Castle.Error`, while an exception, throw or exit the operation did not handle
279+
is let out unchanged - so a rescue narrowed to `Castle.Error` will miss a
280+
command that blew up.
274281

275282
Every command is documented with the `bin/castle` command that reaches it -
276283
`releases`, `upgradable`, `unpack`, `install`, `commit`, `remove` - and with
277284
what it refuses and what it leaves behind: that an install is provisional until
278-
it is committed and a restart until then returns to the previous version, that
279-
`bin/castle install` confirms the version is running rather than trusting the
280-
reply, that removing a version deletes what nothing else is using, and that the
281-
two questions - `upgradable` and `releases` - answer on a deployment where
282-
everything else is refused. `Castle.running/1` is documented too: it has no
283-
`bin/castle` command of its own, and it is what automation driving an upgrade
284-
over `rpc` needs in order to confirm one.
285-
286-
Every public function now carries a `@spec`. `Castle.make_releases/0` is
287-
deliberately not published: its only caller is the launcher, on the first start
288-
of a deployment, and by hand it either does nothing or does what the next start
285+
it is committed, and what that does and does not promise (an ordinary restart
286+
returns to the previous version, but the reboot a restart transition has
287+
already asked for boots the target); the five steps an install takes before
288+
`:release_handler.install_release/1` is asked for anything, in the order it
289+
takes them; that `bin/castle install` confirms the version is running rather
290+
than trusting the reply; that `bin/castle commit` with no version commits the
291+
release awaiting commit and fails when there is none, and that an explicit
292+
commit re-expands the target's configuration even for a version that is
293+
already permanent; that removing a version deletes what nothing else is using;
294+
and that the two questions - `upgradable` and `releases` - answer on a
295+
deployment where everything else is refused. `Castle.running/1` is documented
296+
too: it has no `bin/castle` command of its own, and it is what automation
297+
driving an upgrade over `rpc` needs in order to confirm one.
298+
299+
Every public function now carries a `@spec`, every arity of them:
300+
`Castle.install/1` through `install/5` are one definition with defaults, so
301+
they are five specs rather than two. `Castle.make_releases/0` is deliberately
302+
not published: its only caller is the launcher, on the first start of a
303+
deployment, and by hand it either does nothing or does what the next start
289304
would do anyway.
290305

291306
### Changed

0 commit comments

Comments
 (0)