From 320818f854617b496238a06d8ce6524656d150ed Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 02:28:48 +0000 Subject: [PATCH 1/4] docs: record the failure modes the audit found, and the rules that prevent them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The eight theme pull requests fix the instances. These are the rules, so the next instance — which will be in a file none of those touch — does not get written in the first place. docs/failure-modes.md groups them by the shape of the mistake rather than by subsystem, because that is how they recur, and gives each one the concrete failure that motivated it. AGENTS.md carries the short form, since that is what gets read before every change. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HtxQdsF6YhLzz9RrDmHEDv --- AGENTS.md | 14 +++ docs/failure-modes.md | 201 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 215 insertions(+) create mode 100644 docs/failure-modes.md diff --git a/AGENTS.md b/AGENTS.md index 48c34357..7ee45511 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -35,4 +35,18 @@ - NEVER edit or delete an existing migration block in `crates/core/src/runtime/db.rs`. Once a migration has shipped, its schema_version row exists in real databases and that block will never run again. Editing it silently diverges the live schema from what the version number promises. Always add a new `version < N` block at the bottom. - Unless explicitly excluded, everything available in the OI and the web UI should have a CLI command. - Update docs and guides in docs/ when making changes. +- docs/failure-modes.md lists the ways this codebase has repeatedly gone wrong, each with the rule that prevents it. Read it before working on the reconciler, the fault store, replay, or anything that retries. The rules below are its short form. +- Never let "could not determine" become a definite answer. A failed query, an unreadable row, an unparseable name, or an action you deliberately skipped must not be reported as absence, as non-ownership, or as success. If failure and a definite negative produce the same value, split the type so the compiler forces the caller to decide. +- On any failure path, ask what remains observable. A function returning a value alongside an error must never let that value replace a previous good one; a handler mutating both memory and the database commits durably before the change becomes observable; every early `return Err` leaves both untouched. +- State that is rebuilt in full and applied wholesale (routes, nftables rules, proxy config) must not be applied when a contributor is missing from it — absent is indistinguishable from deleted. An apply that was withheld must not then clear the fault for what it did not do. +- Never match a runtime identity by the shape of its name. `starts_with` or SQL `LIKE 'x%'` on a unit, container, network, volume or ingress name is a red flag: names are not prefix-free. Use a prefix scan only to enumerate candidates, then match exactly against recorded identity. +- Names the daemon grants itself in an operator-shared namespace belong in `crates/core/src/reserved.rs`, are rejected at creation only (never on update or delete), and their destructive consumers additionally check recorded ownership. Derivation is not allocation: if uniqueness matters, allocate and record it. +- Every fault kind names both when it is filed and when it clears, and which shape it is (condition / event / latched). File through `file_once` or `sync_faults`, key it `(app, kind, subject)` where subject is the faulty thing rather than a substring of the description, and clear no more broadly than you file. +- Every loop containing a fallible await names its back-off source, its transient/fatal classification, and its exit-reporting path. No terminal give-up state without an expiry or an operator reset — past a threshold, file a fault and keep retrying at the cap. "Retry immediately" and a bare `_ => break` are the tells. +- A subsystem with a central decision function admits no dispatch before the decision. +- Values crossing the BSL boundary are converted with `defs::take::*` or an equivalent throw. Never coerce, default, or silently drop malformed script input — it changes what the script means rather than failing. +- For anything persisted for crash recovery, answer four questions: who else writes this row, which restart path reads it, what happens on every abort branch of that path, and which test severs in-memory state between the write and the read. Replay matches the committed entry at its own position, never a similar-looking one elsewhere. +- Never `INSERT OR REPLACE` a row another writer touches: it resets every column you do not name, and an `ALTER TABLE ADD COLUMN` can make a correct statement wrong later. Use `ON CONFLICT ... DO UPDATE` naming your own columns. +- When several call sites implement the same wire contract, that is the bug — extract it. Per-caller variation belongs after the handshake, not inside it. +- `etc/ci/` holds greps enforcing some of the above. If one fires, fix the code or justify the exception in the script's allowlist with a comment; do not widen the pattern. diff --git a/docs/failure-modes.md b/docs/failure-modes.md new file mode 100644 index 00000000..ae23f05b --- /dev/null +++ b/docs/failure-modes.md @@ -0,0 +1,201 @@ +# Failure modes + +Recurring ways this codebase has gone wrong, and the rule that prevents each. + +These come from the [July 2026 logic-bug audit](logic-bug-audit-2026-07.md), which found +the same handful of mistakes repeated across unrelated subsystems. They are grouped by the +shape of the mistake rather than by subsystem, because that is how they recur: the next +instance will be in a file none of these examples touch. + +Each entry gives the rule, the concrete failure that motivated it, and what to look for +while writing or reviewing. Where a rule is mechanically checkable there is a script under +`etc/ci/` and a spec item under `docs/spec/`. + +> The helpers, guard scripts and spec items named below (`reserved.rs`, `defs::take`, +> `runtime::retry`, `file_once`/`sync_faults`, `etc/ci/check-*.sh`, and the `r[…]` / `i[…]` / +> `l[…]` references) land with the eight theme pull requests that close the audit, and with +> the audit itself. Until those merge, treat the references as the intended destination +> rather than as things you will find in the tree. The rules hold either way. + +--- + +## 1. "Could not determine" is not an answer + +**Never let a failed query, an unreadable row, an unparseable value, or a skipped action +become a definite result.** + +This is the single most repeated defect in the audit, and it kept reappearing *in the fixes +for it* during review. Every instance looks locally reasonable and is destructive in +aggregate: + +- A failed podman/systemd probe returned an observation with every flag false — byte-for-byte + what "confirmed absent" looks like. The Job terminal-detection predicate read that as + "naturally finished", stopped an in-flight batch workload and recorded it as completed, from + which `job-terminal.defense` guarantees it is killed again if it reappears. +- An unrecognised container state mapped to "container missing" *in an arm that had already + proven the container exists*, so a container draining through its stop timeout was recorded + as removed and barriers over its volumes released early. +- A failed database read produced an empty expected-unit set, which filtered every unit away + and read as "teardown finished", deleting the registry rows while the units were still loaded. +- A failed ownership lookup was treated as "not owned" by a destructive startup sweep. +- A withheld apply returned `Ok(())`, which the caller could not distinguish from a successful + one, so it *cleared* the fault for the thing it had deliberately not done. + +**Check:** for every error branch, ask what the caller will conclude from the value you return. +If failure and a definite negative produce the same value, the type is wrong — split them +(`Observed`/`Failed`, `Some(Result)`/`None`) so the compiler forces the caller to decide. +Absence *confirmed by a successful query* is real evidence and must keep working; it is only +the unknown case that must not masquerade as it. + +Spec: `r[observe.failure-not-absence]`, `r[reconciliation.absolute-state]`. + +## 2. On failure, observable state is unchanged + +**A failure path must leave the world as it found it, or as the last good state.** + +`evaluate_script` returns an `App` populated up to wherever the script threw. That contract is +right for registration, where no previous definition exists, and was applied unchanged to +reload — so a typo halfway down a script published a truncated definition, and the post-update +diffs read it as "the operator deleted everything below this line": volume data relocated, +scaling decisions wiped, forwards torn down, schedules pruned. One `throw`. + +**Check:** + +- A function returning a value alongside an error (`(T, Option)`): may that `T` ever replace + a previous good one? If the answer differs by caller, return an enum, not a tuple. +- A handler that mutates memory and the database: commit durably *before* the change becomes + observable, and make sure every early `return Err` leaves both untouched. A half-registered + app that `/apps/list` shows and a restart drops is worse than a clean failure. +- State that is rebuilt in full and applied wholesale — routes, nftables rules, proxy config — + must not be applied when a contributor is missing from it. Absent is indistinguishable from + deleted, and the containers are still running. + +Spec: `i[app.update]`, `i[app.register]`, `r[reconciliation.absolute-state]`. + +## 3. Match identities against the record, not the shape of a name + +**`starts_with`, or SQL `LIKE 'x%'`, on a unit, container, network, volume or ingress name is +a red flag.** + +Uninstall recognised an app's units with `seedling-{app}-`. Both app names and resource names +may contain hyphens, so the encoding is not prefix-free: uninstalling `app` matched every unit +of a sibling called `app-db` and stopped them, every tick, while never completing. The exact +identities were already in `resource_instances` the whole time. + +A prefix scan is fine to *enumerate candidates*; the decision must be an exact match against +recorded identity. + +The same discipline at grant time: names the daemon takes in an operator-shared namespace +(`backup-snap-*` volumes, the `tailscale` ingress) belong in `crates/core/src/reserved.rs`, +are rejected **at creation only** — an operator must still be able to rename or remove +something that predates the reservation — and their destructive consumers additionally check +recorded ownership, because reservation cannot repair a collision that already exists. + +Derivation is not allocation. Deriving a pod subnet from eight bits of an instance id gave +every static Job the same one, because their ids are nil. If uniqueness matters, allocate and +record the allocation. + +Spec: `r[app.uninstall.scope]`, `r[namespace.reserved]`, `r[infra.pod.subnet]`. + +## 4. Every fault names both halves of its lifecycle + +**A fault kind that does not say when it clears is not finished.** + +Dedup, subject identity, and the clearing condition were re-implemented at every call site and +had diverged into five idioms. The results: a fault filed once per event without bound and +never cleared; faults that could never clear after a restart because the "previously seen" set +was in memory; and a successful backup of one volume clearing every *other* volume's failure. + +**Check:** + +- Identity is `(app, kind, subject)` — the subject is the *thing* that is faulty, never a + substring of the description. File through `file_once` or `sync_faults`. +- **Clear no more broadly than you file.** A kind-wide clear for a subject-keyed fault is + exactly the bug above. +- Say which shape it is: a *condition* fault (true right now — converge it against the + database each tick, which is restart-safe by construction), an *event* fault (this happened — + dedup on file, and name the clear path explicitly), or *latched* (deliberately outlives its + trigger — name the lifecycle event that clears it). +- The file set and the clear set for one tick must be disjoint per key. + +Spec: `r[fault.lifecycle]`. + +## 5. Every retry needs a back-off and a way back + +**A loop containing a fallible await names three things: its back-off, its transient/fatal +classification, and its exit-reporting path.** + +Each audited site had at most one. Image pulls retried on every 5 s tick and then set an +`exhausted` flag nothing could clear, because entries were removed only on a success that could +no longer be attempted — a briefly-unreachable registry disabled the workload until the daemon +restarted. A UDP relay treated a legal zero-length datagram and an ICMP port-unreachable as +fatal and exited silently, leaving the forward listed as healthy with nothing behind it. + +**Check:** + +- No terminal give-up state without an expiry or an operator reset path. Past a threshold, + escalate to a fault and keep attempting at the cap — the fault can only clear because you + kept trying. +- `"retry immediately"` and a bare `_ => break` are the tells. +- Both ends of one protocol must classify errors identically. They did not, and one end killed + forwards the other end merely dropped a datagram on. +- **A subsystem with a central decision function admits no dispatch before the decision.** TLS + issuance dispatched one provider ahead of the state machine and so ignored the operator block + and the failure debounce — and the resulting attempt-row flood evicted *other* hostnames' + debounce state. + +Use `runtime::retry::RetryGate` for tick-driven pacing rather than hand-rolling counters. +Spec: `r[actuate.image.retry]`, `i[forward.relay.resilience]`. + +## 6. Throw at the boundary; never coerce + +**A value crossing the BSL boundary is converted with `defs::take::*` or an equivalent throw.** + +`into_string().unwrap_or_default()`, `filter_map(try_cast)` and bare `as` casts do not fail on +a script type error — they change what the script *means*, far from the cause. +`select(#{ types: ResourceType.Service })` with the brackets forgotten dropped the criterion, +and a `select` with no surviving criteria matches every resource in the app, so a following +`rt.stop` stopped every workload. `pids_limit(4294967297)` wrapped to 1. + +Genuine type *dispatch* that ends in a throw is fine; silent defaulting is not. + +Enforced by `etc/ci/check-defs-coercion.sh`. Spec: `l[bsl.args.strict]`. + +## 7. Anything persisted for crash recovery answers four questions + +**Who else writes this row? Which restart path reads it? What happens on every abort branch of +that path? Which test severs in-memory state between the write and the read?** + +If the last question has no answer, the change is not done — every finding in this class had a +write path and a read path that were each tested alone. + +- **Replay is positional.** A replayed call matches the committed entry at *its own position*, + never a similar-looking one elsewhere. Scanning by value swallowed a second identical call + and re-delivered one whose arguments had shifted. Validate arguments that are stable by + construction (a literal in the script); do not validate ones that legitimately vary between + passes (a resolved instance set). +- **Never `INSERT OR REPLACE` a row another writer touches.** It is delete-then-insert: every + column you do not name reverts to its default. It was correct when written and became wrong + when a migration added `cancel_requested`, silently dropping cancellations. Use + `ON CONFLICT ... DO UPDATE` naming your own columns, so a future `ALTER TABLE ADD COLUMN` is + safe by default. +- **Stamp an effect when it happens, not when it is intended.** A queued schedule fire was + recorded as fired while it existed only in an in-memory queue, so a restart lost it and the + catch-up guarantee had nothing to catch up on. + +Enforced by `etc/ci/check-insert-or-replace.sh`. Spec: `r[barrier.replay.positional]`, +`r[history.persist.partial-update]`. + +## 8. A wire contract belongs in one place + +**When several call sites implement the same protocol handshake, that is the bug.** + +Four consumers hand-rolled the subscription handshake and each got a different part wrong: two +parked forever on an error response, one reported a rejection as success. The happy path works +on first test and the error path is invisible until a transient `server_busy` bricks a +long-lived consumer. + +The per-caller variation is what happens *after* — retry policy, exit code, how the error is +rendered. The handshake itself is not per-caller. + +Enforced by `etc/ci/check-accept-uni.sh`. Spec: `i[stream.subscribe]`. From fbb930b92f145a762039310fd364aa7ad226e989 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 02:31:41 +0000 Subject: [PATCH 2/4] docs: link the audit by PR so the reference survives merge order Review catch: the relative link breaks if this merges before the audit. --- docs/failure-modes.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/failure-modes.md b/docs/failure-modes.md index ae23f05b..709b3a57 100644 --- a/docs/failure-modes.md +++ b/docs/failure-modes.md @@ -2,7 +2,9 @@ Recurring ways this codebase has gone wrong, and the rule that prevents each. -These come from the [July 2026 logic-bug audit](logic-bug-audit-2026-07.md), which found +These come from the July 2026 logic-bug audit +([PR](https://github.com/beyondessential/seedling/pull/115); once merged, also +[in tree](logic-bug-audit-2026-07.md)), which found the same handful of mistakes repeated across unrelated subsystems. They are grouped by the shape of the mistake rather than by subsystem, because that is how they recur: the next instance will be in a file none of these examples touch. From 63fc49d313f0d66082bc4cf2859d30c97df142fd Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 03:29:26 +0000 Subject: [PATCH 3/4] docs: do not link a file the tree does not have yet The audit lands in a separate PR, so the in-tree reference was a dangling link on main until then. Name the path instead of linking it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HtxQdsF6YhLzz9RrDmHEDv --- docs/failure-modes.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/failure-modes.md b/docs/failure-modes.md index 709b3a57..576fac40 100644 --- a/docs/failure-modes.md +++ b/docs/failure-modes.md @@ -3,8 +3,8 @@ Recurring ways this codebase has gone wrong, and the rule that prevents each. These come from the July 2026 logic-bug audit -([PR](https://github.com/beyondessential/seedling/pull/115); once merged, also -[in tree](logic-bug-audit-2026-07.md)), which found +([PR](https://github.com/beyondessential/seedling/pull/115); once that merges, +in tree at `docs/logic-bug-audit-2026-07.md`), which found the same handful of mistakes repeated across unrelated subsystems. They are grouped by the shape of the mistake rather than by subsystem, because that is how they recur: the next instance will be in a file none of these examples touch. From 6286480148a5df9b4364566b2d70c61974cc2779 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 05:04:16 +0000 Subject: [PATCH 4/4] docs: make the failure modes a living document MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every reference to the pull requests that introduced it — the audit and the eight theme changes — rots the moment they merge, leaving a document that reads as a snapshot of one week's work rather than as standing guidance. Drop them, and say instead what earns a ninth entry. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HtxQdsF6YhLzz9RrDmHEDv --- docs/failure-modes.md | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/docs/failure-modes.md b/docs/failure-modes.md index 576fac40..de92f651 100644 --- a/docs/failure-modes.md +++ b/docs/failure-modes.md @@ -2,22 +2,19 @@ Recurring ways this codebase has gone wrong, and the rule that prevents each. -These come from the July 2026 logic-bug audit -([PR](https://github.com/beyondessential/seedling/pull/115); once that merges, -in tree at `docs/logic-bug-audit-2026-07.md`), which found -the same handful of mistakes repeated across unrelated subsystems. They are grouped by the -shape of the mistake rather than by subsystem, because that is how they recur: the next -instance will be in a file none of these examples touch. +The first eight came from a whole-codebase logic-bug audit, which found the same handful of +mistakes repeated across unrelated subsystems. They are grouped by the shape of the mistake +rather than by subsystem, because that is how they recur: the next instance will be in a +file none of these examples touch. Each entry gives the rule, the concrete failure that motivated it, and what to look for while writing or reviewing. Where a rule is mechanically checkable there is a script under `etc/ci/` and a spec item under `docs/spec/`. -> The helpers, guard scripts and spec items named below (`reserved.rs`, `defs::take`, -> `runtime::retry`, `file_once`/`sync_faults`, `etc/ci/check-*.sh`, and the `r[…]` / `i[…]` / -> `l[…]` references) land with the eight theme pull requests that close the audit, and with -> the audit itself. Until those merge, treat the references as the intended destination -> rather than as things you will find in the tree. The rules hold either way. +Add to this document when a mistake turns out to be a class rather than a one-off — when +the same shape shows up in a second unrelated subsystem, or when a fix for one instance +reintroduces it somewhere else. A rule earns its place by naming what to look for, not by +recounting what went wrong. --- @@ -26,9 +23,8 @@ while writing or reviewing. Where a rule is mechanically checkable there is a sc **Never let a failed query, an unreadable row, an unparseable value, or a skipped action become a definite result.** -This is the single most repeated defect in the audit, and it kept reappearing *in the fixes -for it* during review. Every instance looks locally reasonable and is destructive in -aggregate: +This is the most repeated defect of the lot, and the one that most readily reappears *in the +fixes for it*. Every instance looks locally reasonable and is destructive in aggregate: - A failed podman/systemd probe returned an observation with every flag false — byte-for-byte what "confirmed absent" looks like. The Job terminal-detection predicate read that as @@ -127,11 +123,12 @@ Spec: `r[fault.lifecycle]`. **A loop containing a fallible await names three things: its back-off, its transient/fatal classification, and its exit-reporting path.** -Each audited site had at most one. Image pulls retried on every 5 s tick and then set an -`exhausted` flag nothing could clear, because entries were removed only on a success that could -no longer be attempted — a briefly-unreachable registry disabled the workload until the daemon -restarted. A UDP relay treated a legal zero-length datagram and an ICMP port-unreachable as -fatal and exited silently, leaving the forward listed as healthy with nothing behind it. +Every site that got this wrong named at most one. Image pulls retried on every 5 s tick and +then set an `exhausted` flag nothing could clear, because entries were removed only on a +success that could no longer be attempted — a briefly-unreachable registry disabled the +workload until the daemon restarted. A UDP relay treated a legal zero-length datagram and an +ICMP port-unreachable as fatal and exited silently, leaving the forward listed as healthy +with nothing behind it. **Check:**