Skip to content

feat(restore): one restore check per server, replicas as instances - #482

Merged
passcod merged 4 commits into
mainfrom
feat/restore-check-instances
Aug 4, 2026
Merged

feat(restore): one restore check per server, replicas as instances#482
passcod merged 4 commits into
mainfrom
feat/restore-check-instances

Conversation

@passcod

@passcod passcod commented Aug 4, 2026

Copy link
Copy Markdown
Member

🤖 Completes the check-name work started in #480. Those three checks were the remaining parameterised names.

Why

restore-verification, redaction and migration-test were filed under names carrying (type, intent)restore-verification:tamanu-postgres:verify. Both dimensions are open-ended strings (BackupType::Custom(String), RestoreIntent(pub String)), so the catalog fanned out without bound: three conditions became three × types × intents entries to configure. Worse than the backup case #480 fixed, which had one open dimension.

They are now one check each per server, with each replica an instance graded on its own. Type, intent and the replica's name live in the filing's detail, where policy rules reach them.

The derivation rule

This is the part worth reviewing. A replica key (server, type, intent) yields an instance when any of:

  1. An enabled declaration covers it on that server — whatever the consumer currently advertises. Capability and semantic checks were removed from instance existence and kept only where they're genuinely needed, computing overdue. An unadvertised intent is a gap to surface, not grounds to drop a standing finding.
  2. A report exists for it and an enabled declaration still asks for that replica somewhere in the server's group. So a report about a server the declaration doesn't name still surfaces against that server.
  3. A recorded migration verdict exists for it, ungated — so a verdict with replica_id: None and no declaration anywhere still surfaces.

An instance's result is the worse of its latest report and its overdue bound. Those used to be two writers racing on one check name.

Two bugs fell out of getting this right: a check that is open but derives no instances is now recovered rather than left open forever when the last replica goes away, and servers with open checks that derive nothing are visited at all.

Where the agent's brief was wrong, and it said so

I briefed this as "a key with a report but no declaration still yields an instance". That cannot coexist with the four tests whose guarantees I also said must hold. Under the literal rule, a deleted or disabled declaration leaves a failed report as a permanently derivable instance that nothing can clear — the ingest only accepts reports an enabled declaration authorizes, so once nothing declares the replica, no report can ever arrive to recover it. Rule 2 above is the closest thing to the intent that doesn't manufacture unrecoverable findings.

Two fixtures changed as a consequence, neither by loosening an assertion:

  • three record_report_* tests now declare the replica they report on — a report with no declaration at all is a state production cannot reach, so those fixtures described an impossible world;
  • re_enabling_does_not_recover_anything records its report while the declaration is enabled, then disables, then re-enables. Its middle assertion contradicted disabling_recovers_the_stale_alert head-on; a stateless re-derivation cannot distinguish a report that landed while the replica was decommissioned from one that landed before. The guarantee in the name still holds.

The two-parameter silence guard does not fit the DSL

Condition is one operator over one variable and IfLadder has no and/or, so check.type == X AND check.intent == Y is inexpressible. Widening a silence to every replica of a type, or dropping it, were both unacceptable — an operator's stored suppression has to survive exactly. So every instance carries check.replica_key = "<type>:<intent>" and the migration guards on that. Composite identity in the detail, not in the name, so the rule from #480 holds. Documented in the migration, the sweep, CHK, and the shipped check documentation.

Migration

collapse_restore_check_names mirrors #480's across the three prefixes: issues collapse onto one survivor per target (losers leave their incidents; emptied incidents retire), silences become check.replica_key guards, catalog rows collapse preferring an operator-reviewed one. Verified against synthetic data — two servers, colliding pairs, an incident held only by losers, a pre-existing bare migration-test catalog row, and an unrelated colon-bearing check — in a rolled-back transaction.

Operator-visible

  • One check of each kind per server; stored silences rewritten to per-replica guards.
  • A finding appears or clears on the next sweep (minute cadence) rather than at ingest. These are non-paging warnings, so the delay is immaterial; the gain is one filer instead of two that drift.
  • A withdrawn capability no longer silently clears a finding. A deleted, disabled or rescoped declaration still does.
  • A failed migration test raises the check with no declaration in sight — which it did via the ingest path before, and briefly stopped doing mid-change.
  • Servers with declarations but no findings no longer get passing rows or catalog entries seeded until something degrades.

Follow-up noted, not done

latest_by_key and latest_verdict_by_key are fleet-wide DISTINCT ON scans each pass. The existing index is (server_id, type, observed_at DESC), so Postgres sorts rather than walking it. Fine at current volume; an index on (server_id, type, intent, observed_at DESC) is the obvious fix if backup_restore_checks grows.

Also: two declarations covering one key (group-wide plus server-scoped) produce one instance labelled by the first by (name, id), with their overdue verdicts OR'd.

Commits

The first commit is deliberately the broken intermediate — it derives instances from declarations only — with the fix layered on top rather than squashed in, so the two are revertible apart. Its message says which part is wrong.

@passcod
passcod added this pull request to the merge queue Aug 4, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Aug 4, 2026
passcod added 4 commits August 4, 2026 17:11
restore-verification, redaction and migration-test were filed under
names carrying (type, intent) — restore-verification:tamanu-postgres:
verify and so on. Both dimensions are open-ended strings (BackupType::
Custom, RestoreIntent(String)), so the catalog fanned out without bound:
three checks became three times types times intents entries to
configure, for three conditions.

They are now one check each per server, with each replica an instance
graded on its own, filed through file_check_instances. Type, intent and
the operator's replica name live in the detail, where policy rules reach
them, so a rule or silence for one replica applies to only that replica.

sweep_overdue becomes their sole filer, which removes a race: overdue
and report-health were two writers on the same check name. It also
closes a gap — the sweep was filtered to overdue_after IS NOT NULL, so a
declaration without a bound only ever got a check from the ingest path.
And roughly seventy lines of hand-rolled recover_old_scope_alerts go
away, since a deleted or rescoped declaration now simply stops being one
of the server's instances.

The instance set this commit derives is wrong — it comes only from live
declarations, which is stricter than what the ingest path required, so
findings vanish when a capability is withdrawn and migration verdicts
with no declaration link never surface at all. The next commit fixes
that; this one is kept separate so the two are revertible apart.
Regression tests for the two ways a finding used to vanish: a consumer
withdrawing the capability its replica was declared under, and a
migration verdict recorded with no declaration link at all. Plus a
report about a server its declaration does not name, which surfaces
against that server and goes when nothing declares the replica any more.

One server with several replicas holds one check of each kind, naming
the degraded ones with per-replica results in the detail, and the
catalog gains one entry per check rather than one per (type, intent).

The three record_report tests now declare the replica they report on:
the ingest only accepts a report an enabled declaration authorizes, so a
report standing on its own was a state production cannot reach, and the
fixture said otherwise. re_enabling_does_not_recover_anything records
its report while the declaration is enabled for the same reason — a
re-derivation cannot tell a report that landed while the replica was
decommissioned from one that landed before, and the guarantee in the
name is that putting the replica back does not clear what it reported.

The specs say how a replica is derived, that a redaction check is one
per server with instances, and that an instance identified by more than
one field carries them joined, since a rule condition matches a single
variable.
It stopped being about overdue when it became the sole filer of
restore-verification, redaction and migration-test: it files what the
latest report said and what a verdict recorded, and overdue is one of
three ways an instance is degraded. The shipped documentation for the
three checks says the same, and points an operator at check.replica_key
for handling one replica differently from the rest.

The two restore-replica endpoints' docs no longer promise a recovery at
the moment the declaration changes — it lands on the next sweep — so the
private-server openapi and the types generated from it come along.
@passcod
passcod force-pushed the feat/restore-check-instances branch from 758e8dc to 086c413 Compare August 4, 2026 05:16
@passcod
passcod added this pull request to the merge queue Aug 4, 2026
Merged via the queue into main with commit ba544ee Aug 4, 2026
7 checks passed
@passcod
passcod deleted the feat/restore-check-instances branch August 4, 2026 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant