feat(faults): one fault identity and a shared file/clear discipline (audit theme 4)

[passcod]

Closes cross-cutting theme 4 from the logic bug audit: fault lifecycle asymmetries. See the pattern analysis.

The class

The fault store was deliberately minimal — file_fault a bare INSERT, clear_fault a flag flip — so dedup, subject identity, and when a fault stops being true were re-implemented at every call site. Five idioms had diverged:

• Ad-hoc dedup scans, each picking its own identity: (kind, instance_id), (kind, resource_name), (kind, description), bare kind — and audit_lag skipping the scan entirely, so it duplicated without bound.
• In-memory prev-set diffs: reconcile_ingress_conflicts and reconcile_site_service_faults cleared only prior \ current, where prior is a Reconciler field that starts empty on every daemon start. The faults are in the database, so one filed before a restart could never clear.
• Over-broad clears: any volume's backup success called clear_faults_by_kind(app, "backup_failed"), wiping every other volume's fault.
• Missing halves: audit_lag had no clear path; tailscale_unreachable had a threshold constant and a doc comment but no filing code at all.

Five of the six findings are not wrong judgements but wrong bookkeeping, copy-pasted per site.

The change

Faults get a first-class identity — (app, kind, subject), where subject is the thing that is faulty: a volume id, an image ref, a host:port, an instance hex. It used to hide in resource_name, in instance_id, or in a substring of the description, and where none of those fitted it was simply absent, which is why clearing had to fall back to app-plus-kind.

• Migration v53 adds the column, backfills it from instance_id/resource_name, clears pre-existing duplicates (keeping the newest of each group — the migration would otherwise fail to build its index over them), and adds a partial unique index enforcing at most one active fault per key.
• file_once(db, key, meta, description) — the dedup four sites hand-rolled. Enforced by the index rather than a read-then-write, so concurrent filers cannot both win.
• sync_faults(db, scope, current) — converge a scope to the set of conditions that hold right now: file what is missing, clear what is no longer present. Restart-safe by construction, because it compares against the persisted active set rather than an in-memory prior one. FaultScope bounds what a sweep may clear, so converging one kind cannot touch another.

file_fault keeps its signature and derives a subject the same way the migration backfills, so the ~20 sites not ported here get the uniqueness guarantee without a change.

Findings closed

Finding	Severity	How
H8 — a later volume's success erases an earlier volume's backup_failed	high	keyed by volume; success clears only its own
ingress_conflict and site-service faults never clear after a daemon restart	medium	both prev-set diffs are now sync_faults sweeps; the two Reconciler fields are deleted
stop_failed filed and cleared in the same tick	medium	stop_sent is recorded before the stop is attempted, so the file and clear sets overlapped; they are now disjoint per instance
Adding a registry leaves stale disallowed_registry faults	medium	/registries/add re-evaluates, as /registries/remove already did
audit_lag filed without dedup and never cleared	low	file_once, with the clear path named at the site
Promised tailscale_unreachable fault never filed	low	filed and converged on the consecutive-failure threshold

Enforcement

• Spec: new r[fault.lifecycle] in runtime.md, stating the what: every kind defines both its filing and its clearing condition; identity is the faulty thing, not just the app; at most one active fault per key; clearing keyed no more broadly than filing; a condition fault is active exactly while its condition holds, including across restarts; a latched fault names the event that clears it.
• Tests: file_once does not duplicate; two subjects of one kind clear independently (H8); a sweep clears a fault pre-seeded as if by a previous process, with no in-memory memory of it (the restart case); sync_faults is idempotent and converges from any starting state; a sweep never touches kinds outside its scope; and a TestOi test that /registries/add clears the fault it resolves.

Two existing tests changed: they filed two faults of the same kind for one app with no subject and asserted both stayed active. Under the new invariant that is one fault, so they now use distinct subjects — the behaviour they were testing (kind-wide clear, per-app count) is unchanged.

Not in scope

Computing the wrong condition in the first place — the healthcheck-replace target bug in §12 would survive any amount of fault plumbing. Latched kinds (crash_loop, health_check_replace_failed) keep their semantics: they deliberately outlive their trigger and must not be converged to "currently observed". The remaining file_fault sites are correct as they stand and inherit the uniqueness guarantee; porting them to explicit subjects is mechanical follow-up.

Overlap with other themes

Theme 3 (observation uncertainty) stacks on this one — it gates the observe_failed fault on consecutive failures, in the same reconcile/faults.rs and reconcile/pods.rs this PR edits. Independent of the rest.

---

Generated by Claude Code

[Copilot]

Pull request overview

This PR implements the “fault lifecycle” discipline described in the audit theme 4 write-up by giving faults a first-class identity (app, kind, subject) and adding shared helpers to file-once and converge active faults against the persisted set (restart-safe), then ports several key call sites to the new approach.

Changes:

• Adds DB migration v53 introducing faults.subject and a partial unique index enforcing at most one active fault per (app, kind, subject), including backfill and duplicate cleanup.
• Introduces FaultKey, FaultMeta, file_once, FaultScope, and sync_faults, and updates multiple subsystems (reconciler, backups, audit, tailscale, registries) to use converge/narrow clears.
• Updates spec and expands tests to lock in the new lifecycle invariants and restart-safe clearing behaviour.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
docs/spec/runtime.md	Adds the new r[fault.lifecycle] spec invariant describing identity, uniqueness, and restart-safe clears.
crates/core/src/system/reconcile/faults.rs	Ports ingress-conflict + site-service fault handling to persisted-set convergence via sync_faults.
crates/core/src/system/reconcile.rs	Removes in-memory “prior set” fields that prevented clears after restart.
crates/core/src/runtime/tailscale.rs	Implements filing/clearing for tailscale_unreachable as a converged condition fault.
crates/core/src/runtime/faults/tests.rs	Updates existing tests for subject uniqueness and adds new lifecycle/convergence tests.
crates/core/src/runtime/faults.rs	Adds subject, introduces FaultKey/FaultScope, implements file_once + sync_faults, and updates core fault DB operations.
crates/core/src/runtime/db/tests.rs	Bumps expected schema version to 53.
crates/core/src/runtime/db/migrations/v53.sql	Adds subject, backfills it, clears pre-existing duplicates, and adds partial unique index for active faults.
crates/core/src/runtime/db.rs	Registers migration v53.
crates/core/src/runtime/audit.rs	Switches audit_lag to file_once to prevent unbounded duplication.
crates/core/src/oi/handler/registries/tests.rs	Adds an OI test ensuring /registries/add clears disallowed_registry when it resolves.
crates/core/src/oi/handler/registries.rs	Re-evaluates apps after adding a registry so condition faults clear promptly.
crates/core/src/oi/handler/backups.rs	Keys backup faults by volume and clears only the relevant volume’s faults on success.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

crates/core/src/runtime/db/migrations/v53.sql:20

• Migration v53 only backfills subject for active faults (WHERE cleared_at IS NULL ...). Because the faults table is also used as a history log, this leaves cleared historical rows with an empty subject, making the new identity information unavailable/ambiguous when inspecting past faults (and any future UI/reporting that relies on subject). Backfilling for all rows is safe and keeps history consistent.

UPDATE faults
SET subject = COALESCE(instance_id, resource_name, '')
WHERE cleared_at IS NULL AND subject = '';

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (1)

crates/core/src/oi/handler/backups.rs:633

• clear_backup_faults_for_volume only clears rows where subject == vol_id, but pre-v53 backup faults were filed via file_fault(..., None, None, None, ...), so they backfill to subject == "" and will never be cleared by a later successful backup after upgrade. Also, unwrap_or_default() silently ignores DB errors and can leave faults uncleared with no log.

Consider (a) logging and returning on list_active_faults errors, and (b) clearing legacy empty-subject backup faults alongside the volume-keyed ones so upgrades don’t strand an old backup_failed/backup_source_unavailable fault indefinitely.

fn clear_backup_faults_for_volume(db: &crate::runtime::db::Db, app: &AppName, vol_id: &str) {
    for kind in ["backup_failed", "backup_source_unavailable"] {
        let active: Vec<_> = faults::list_active_faults(db, Some(app))
            .unwrap_or_default()
            .into_iter()
            .filter(|f| f.kind == kind && f.subject == vol_id)
            .collect();

[github-code-quality]

Code Coverage Overview

Languages: TypeScript, Rust

TypeScript / code-coverage/vitest

The overall coverage in commit d0559fc in the claude/pr-115-theme-... branch remains at 66%, unchanged from commit d372037 in the main branch.

Rust / code-coverage/rust

The overall coverage in commit d0559fc in the claude/pr-115-theme-... branch remains at 59%, unchanged from commit d372037 in the main branch.

Show a code coverage summary of the most impacted files.

File	main d372037	claude/pr-115-theme-... d0559fc	+/-
crates/core/src...ntime/faults.rs	98%	92%	-6%
crates/core/src...me/tailscale.rs	45%	40%	-5%
crates/core/src...dler/backups.rs	51%	50%	-1%
crates/core/src...em/reconcile.rs	25%	25%	0%
crates/core/src/oi/server.rs	59%	60%	+1%
crates/core/src...ncile/faults.rs	4%	5%	+1%
crates/core/src...untime/audit.rs	73%	75%	+2%
crates/core/src...r/registries.rs	75%	78%	+3%

---

_{Updated August 02, 2026 02:49 UTC}

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (2)

crates/core/src/oi/handler/backups.rs:633

• Backup faults are now keyed by volume id (subject == vol_id), but existing active backup_failed / backup_source_unavailable rows from before the subject migration were filed with no instance_id/resource_name, so they backfill to an empty subject. Those legacy active faults would never be cleared by a successful backup with the new per-volume clearing logic, leaving a stale condition fault until manual intervention.

        let active: Vec<_> = faults::list_active_faults(db, Some(app))
            .unwrap_or_default()
            .into_iter()
            .filter(|f| f.kind == kind && f.subject == vol_id)
            .collect();

crates/core/src/runtime/faults.rs:131

• The comment says the legacy subject-derivation matches “migration v53”, but this PR introduces the backfill in v55.sql. Keeping the migration number accurate avoids confusion when correlating behaviour to schema versions.

    // Sites that have not yet been given an explicit subject derive one the
    // same way migration v53 backfilled the existing rows, so a fault filed
    // before the migration matches the key its site computes after it.