feat(restore): one restore check per server, replicas as instances - #482
Merged
Conversation
github-merge-queue
Bot
removed this pull request from the merge queue due to a conflict with the base branch
Aug 4, 2026
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.
… only declarations
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
force-pushed
the
feat/restore-check-instances
branch
from
August 4, 2026 05:16
758e8dc to
086c413
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 Completes the check-name work started in #480. Those three checks were the remaining parameterised names.
Why
restore-verification,redactionandmigration-testwere 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:replica_id: Noneand 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:
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_anythingrecords its report while the declaration is enabled, then disables, then re-enables. Its middle assertion contradicteddisabling_recovers_the_stale_alerthead-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
Conditionis one operator over one variable andIfLadderhas noand/or, socheck.type == X AND check.intent == Yis 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 carriescheck.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_namesmirrors #480's across the three prefixes: issues collapse onto one survivor per target (losers leave their incidents; emptied incidents retire), silences becomecheck.replica_keyguards, 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 baremigration-testcatalog row, and an unrelated colon-bearing check — in a rolled-back transaction.Operator-visible
Follow-up noted, not done
latest_by_keyandlatest_verdict_by_keyare fleet-wideDISTINCT ONscans 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 ifbackup_restore_checksgrows.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.