Chosen database states alert once, not forever — edge-trigger OFFLINE/RESTORING (#2166)

[erikdarlingdata]

What changes

The Database State alert re-fired for as long as a database stayed deviated. @gotqn's case: a soft-delete workflow parks a database OFFLINE for a month, which produced hundreds of identical alerts for one intended action.

Now OFFLINE / RESTORING / RECOVERING / STANDBY are edge-triggered — alert on the transition, stay quiet until the state changes. SUSPECT / RECOVERY_PENDING / EMERGENCY are untouched and keep re-firing on the cooldown.

The split is the whole idea: those first states are usually ones somebody chose, and repetition tells the operator nothing new. Nobody parks a database in SUSPECT, so there the repetition is the signal — there's a test pinning that an already-announced integrity state must keep firing, because the failure mode worth guarding is a real corruption quietly stopping its nagging.

Why the memory is persisted, not in-memory

This is the part that decided the design. The case is a database parked for weeks. In-memory edge state would go quiet, then re-announce every parked database on the next service restart — worse than the cooldown-repeat it replaces, and it would look like a regression to exactly the person who asked for this.

So V60 adds last_alerted_state / last_alerted_at to config.database_state_expected — the table that already holds this alert's per-database config, already keyed (server_id, database_name).

Rejected alternative: config_edge_trigger_watermarks already does restart-surviving edge memory, but it's keyed (server_id, metric_name) and metric_name feeds alert history and mute-rule matching. Per-database use would mean smuggling a compound key into a label column — fine today, a trap later.

The composition property @gotqn identified

Pinned by test: a database announced OFFLINE that turns SUSPECT fires again, at Critical. Going quiet for a parked state must not mean going blind, and the new state doesn't inherit the old one's quiet treatment.

Scope

Darling only, matching where the issue was filed. The engine change is inert for Lite by construction: with no persisted memory the adapter reports empty, current != empty is always true, and Lite behaves exactly as before. Lite's SaveDatabaseStateAlertedAsync is a documented no-op rather than a fake in-memory cache — a cache would go quiet and then re-fire on restart, which is the worst of both. Lite parity follows separately, same as #2167 → #2176.

Behavior-change warning

This alters a default. Anyone relying on the repeat-nag to track deliberate offlines will see it go quiet after the first alert — called out in the changelog in those terms.

Testing

Four engine pins: an already-announced chosen state stays silent with no cooldown involved; a first observation fires and records the state (recording is what makes the silence survive a restart); an already-announced SUSPECT still fires at Critical; and OFFLINE→SUSPECT fires again at Critical. Plus the full V60 ladder — rung identity, probe sentinel, gate arm mapping a fully-migrated store to 60 and a V59 store to 59, and the suite's ladder-tip pins moved. Alerting, Darling service, Viewer, Lite, and both test projects build clean.

Addresses #2166 (Darling half).

[claude]

This upsert breaks the "integrity states must keep re-firing" guarantee for the exact case the PR calls out as the composition property to protect.

expected_state is NOT NULL (V49), and this INSERT sets it to $3 = effectiveState — the state that was just alerted, reused for both expected_state and last_alerted_state. That's fine when a row already exists (the ON CONFLICT branch only touches last_alerted_state/last_alerted_at), but it's wrong for a brand-new row.

Concrete sequence:

1. A database with no config.database_state_expected row (never seen deviating before) goes SUSPECT. SeedDatabaseStateExpectedSql deliberately skips seeding SUSPECT/RECOVERY_PENDING/EMERGENCY on first observation ("onboarding a server mid-outage must not learn the bad state as expected... stays pending (no row)").
2. DatabaseStateDeviationsSql's expected_state IS NULL branch matches (two-sample rule), the alert fires at Critical with the "no baseline yet" message.
3. SaveDatabaseStateAlertedAsync now INSERTs a fresh row with expected_state = 'SUSPECT'.
4. Next cycle: e.expected_state is no longer NULL, and l.eff IS DISTINCT FROM e.expected_state is false (both are 'SUSPECT') — the database drops out of the deviation query entirely. No more alerts, ever, even though it's still corrupted.

This directly contradicts DatabaseState_IntegrityState_StillRepeats_EvenWhenAlreadyAnnounced (and the PR's own stated intent that SUSPECT/RECOVERY_PENDING/EMERGENCY must keep nagging on cooldown) — it's just not caught because that test stubs IAlertStateStore and never exercises this SQL against real Postgres.

Since expected_state is NOT NULL, this write path needs to either leave the pending/critical case alone (only stamp last_alerted_state/last_alerted_at, e.g. via a plain UPDATE ... WHERE EXISTS, doing nothing when no row exists yet) or explicitly avoid baselining critical states the same way the seed SQL does.

[claude]

Doc-comment/field mismatch: this V60 /// <summary> block is inserted directly after V59's existing /// <summary> block (lines ~1142-1153, "V59 — the two collector memory knobs...") with no field declaration between them. C# attaches an uninterrupted run of /// lines to whatever member follows, so both summaries merge onto V60Sql below, and V59Sql (declared right after V60Sql) ends up with no doc comment at all. Worth reordering — e.g. put V60's comment+const after V59's comment+const — so each constant keeps its own doc comment, matching how V57/V58/V59 are laid out elsewhere in this file.

[claude]

Reviewed. Solid design overall (edge-triggering only the 'chosen' states, persisting to survive restarts, keeping the integrity states on cooldown) and the engine-level test coverage for AlertEngine.cs is thorough. Two findings, left inline:

1. Correctness (high confidence) — PgAlertStateStore.SaveDatabaseStateAlertedAsync's upsert sets expected_state = effectiveState on INSERT (required since the column is NOT NULL). For a database with no existing config.database_state_expected row that goes SUSPECT/RECOVERY_PENDING/EMERGENCY — the exact "pending, no baseline yet" case SeedDatabaseStateExpectedSql deliberately leaves un-seeded — this baselines the critical state itself as "expected" on first fire. The very next cycle, DatabaseStateDeviationsSql's l.eff IS DISTINCT FROM e.expected_state check is false and the database silently drops out of the deviations list for good. That's the opposite of what DatabaseState_IntegrityState_StillRepeats_EvenWhenAlreadyAnnounced and the PR description both promise (integrity states must keep nagging on cooldown). Not caught by the new unit tests since they stub IAlertStateStore and never touch this SQL.

2. Nit — in PgMigrations.cs, the V60 doc comment is spliced directly between V59's doc comment and V59Sql's declaration, so it merges onto V60Sql and V59Sql ends up undocumented.

No Lite/Darling parity issues — the Lite no-op (LiteAlertStateStore.SaveDatabaseStateAlertedAsync) is explicitly designed and documented as inert-by-construction, matching the PR's stated scope (Darling only, Lite parity to follow separately, same pattern as #2167 → #2176). No SQL-injection or secrets concerns — all writes go through parameterized Npgsql commands.

[claude]

Correctness: this upsert can permanently silence — and falsely "resolve" — a SUSPECT/RECOVERY_PENDING/EMERGENCY database, exactly the case #2166 says must never go quiet.

Walk through a database first observed already critical (no config.database_state_expected row yet — the "pending" path):

1. SeedDatabaseStateExpectedSql (DarlingAlertReadAdapter.cs) deliberately skips baselining SUSPECT/RECOVERY_PENDING/EMERGENCY (... NOT IN ('SUSPECT','RECOVERY_PENDING','EMERGENCY')), specifically so the database stays "pending" and keeps alerting until it recovers or an operator sets an override.
2. Two critical samples in a row → DatabaseStateDeviationsSql's pending branch (e.expected_state IS NULL AND l.eff IN (...) AND p.eff IN (...)) fires the alert.
3. AlertEngine.CheckDatabaseStateAsync calls this method after delivery with effectiveState = "SUSPECT" (say). Because no row exists yet, the INSERT branch runs — and expected_state is NOT NULL, so it gets set to $3, i.e. the current critical state itself.
4. Next cycle: the row now exists with expected_state = 'SUSPECT'. The pending branch no longer matches (expected_state IS NULL is false), and the general branch's l.eff IS DISTINCT FROM e.expected_state is also false (SUSPECT == SUSPECT). The database silently drops out of current in CheckDatabaseStateAsync.
5. Because it drops out of current while still in active, the recovery block (~AlertEngine.cs L1325-1339) treats it as recovered and fires "Database State Resolved" — even though the database is still SUSPECT.
6. It will never alert again unless it later transitions to a different critical state (SeedDatabaseStateExpectedSql never overwrites the row, ON CONFLICT DO NOTHING).

So a real corruption's very first alert both permanently silences the alert and tells the operator it "resolved" — worse than the pre-PR behavior (which just kept re-firing forever), and the opposite of what the PR sets out to guarantee for integrity states.

The doc comment's justification doesn't hold up either: "the baseline column takes the current state in that case, which the seed logic would have written anyway on the next healthy observation" — but the seed logic explicitly refuses to write a baseline for these three states while they remain critical, so this INSERT does something the seed logic never would while the outage is ongoing.

None of the new tests catch this because the in-memory test harness's fake adapter/state-store are decoupled — nothing exercises the real interaction between this INSERT and DatabaseStateDeviationsSql against the same store.

Possible fix: only persist last_alerted_state for the states that actually consult it (DatabaseStateTokens.RepeatsAreNoise), or make this an UPDATE ... WHERE EXISTS so a not-yet-baselined critical database never gets an accidental baseline written.

[claude]

Correctness: recovery (below, ~L1325-1339) never clears the persisted last_alerted_state, so the same "chosen" state recurring in a later, independent episode goes silent forever.

That block clears the in-memory active set and the cooldown entry when a database returns to its expected state, but nothing calls back into _stateStore to clear the persisted last_alerted_state/last_alerted_at this line writes (PgAlertStateStore.cs L172-194).

Concretely: a database is parked OFFLINE → alerts once, last_alerted_state persisted as 'OFFLINE'. Operator brings it back ONLINE → the recovery block fires the "resolved" notice, but the persisted last_alerted_state column is untouched. Weeks later the same database is deliberately parked OFFLINE again (a second, unrelated soft-delete cycle — exactly the workflow described in the PR). DarlingAlertReadAdapter's deviation query still returns last_alerted_state = 'OFFLINE' from the first episode, so alreadyAnnounced (L1262-1264 above) is true and the second parking never alerts at all.

The "composition property" test added here (DatabaseState_ChosenState_ChangingToADifferentState_FiresAgain) only proves re-firing across a different subsequent state (OFFLINE → SUSPECT); nothing pins the same-state-after-recovery case, which is the one most likely to recur in practice for a repeated soft-delete/park workflow on the same database.

Worth clearing the persisted state on recovery (a _stateStore call in the block below), plus a test that pins: park → alert → recover → re-park with the same state → alerts again.

[claude]

Reviewed the diff. Overall the edge-trigger design and the persisted-vs-in-memory reasoning in the PR description are sound, and Lite parity (documented no-op) is correctly inert. Left two inline comments on what I believe are real correctness regressions in the persisted-memory interaction, both missed by the new unit tests because the in-memory test harness's fake adapter/state-store never exercise the real SQL round-trip together:

1. PgAlertStateStore.SaveDatabaseStateAlertedAsync — when a database is first observed already critical (SUSPECT/RECOVERY_PENDING/EMERGENCY, no baseline row yet), the upsert's INSERT branch sets expected_state to the current critical state (it's NOT NULL). On the very next cycle the database then reads as matching its own "expected" state, drops out of the deviation set, and the engine fires a false "Database State Resolved" notification — then never alerts again for that database unless it changes to a different critical state. This is the exact failure mode (SUSPECT going quiet) the PR explicitly says must never happen, and it's worse than pre-PR behavior (infinite repeat) since it now also reports a false recovery.

2. AlertEngine.CheckDatabaseStateAsync — the recovery branch clears the in-memory active/cooldown state when a database returns to its expected state, but never clears the persisted last_alerted_state. So a database parked OFFLINE, brought back online, then independently re-parked OFFLINE weeks later (the literal soft-delete-workflow scenario in the PR body) will never alert on the second parking — last_alerted_state still holds 'OFFLINE' from the first episode. The added "composition property" test only covers transitioning to a different subsequent state, not the same state recurring after a recovery.

Both are edge-memory lifecycle gaps rather than problems with the overall approach — I think both are fixable by (a) only ever persisting last_alerted_state for the states DatabaseStateTokens.RepeatsAreNoise actually consults, and/or guarding the upsert's INSERT branch so it can't invent a baseline for a not-yet-baselined critical database, and (b) clearing the persisted state in the recovery branch.

No Lite/Darling parity issues beyond what's already called out (Lite deliberately inert), and no SQL-injection or secret-handling concerns — all queries are parameterized.

[erikdarlingdata]

Both findings confirmed and fixed in 5e2a74b. Both were real, and the first one was worse than what this PR set out to fix — thank you.

The upsert. Walked the pending path and it lands exactly as described: no row → INSERT → expected_state takes the state being alerted on → the database stops deviating → the recovery block reads it as recovered and fires a false resolution on a still-corrupt database → silent forever. My doc comment's justification ("the seed logic would have written anyway") was simply wrong: SeedDatabaseStateExpectedSql explicitly refuses to baseline SUSPECT/RECOVERY_PENDING/EMERGENCY while they're critical, so this write was doing behind its back the thing it declines to do in front. Now UPDATE-only.

Worth stating why UPDATE-only loses nothing, since "skip the no-row case" sounds lossy: a row is absent only when a database was first observed in an integrity state, and RepeatsAreNoise is false for all three of those, so alreadyAnnounced never consults the memory for them — they keep alerting on the cooldown, which is the intended behavior. The parked-database case that needs the memory always has a row, because it was baselined while the database was still healthy.

The permanent memory. Also correct, and the OFFLINE → restore → OFFLINE-again sequence is the exact workflow the issue describes, so this would have bitten the reporter specifically. Added ClearDatabaseStateAlertedAsync to the interface, both stores and every stub, called on the falling edge. One deliberate choice: it clears even when suppressed, because suppression governs whether operators are told about a transition, never whether the engine keeps accurate state — leaving a stale memory behind under suppression would swallow the next real episode.

On "none of the new tests catch this." That was the most useful part of the review, because it named the structural reason rather than just the miss: the stub state store counted calls while the adapter supplied states, so nothing crossed the round trip the edge trigger actually depends on. FakeStateStore now models what the store would HOLD (save writes, clear removes), and DatabaseState_RepeatEpisode_OfTheSameState_FiresAgainAfterRecovery feeds that memory back in as LastAlertedState for the third evaluation — park, recover, re-park in the same state. It fails if either direction of the trip is missing, which the previous harness could not do. DatabaseState_AlertedStamp_IsAnUpdate_NeverAnInsert pins the SQL so the upsert cannot come back.

Verified: Alerting, Darling.Tests, Lite and Lite.Tests all build clean (the interface addition needed the Lite no-op and three stubs).

[claude]

This clear — the thing that makes the persisted last_alerted_state restart-safe against the exact bug the 3rd commit just fixed — is itself only reachable through _activeDatabaseStateAlerts (line 140), which is in-memory only and resets to empty on every process restart.

ClearDatabaseStateAlertedAsync runs for active.Where(d => !current.ContainsKey(d)) — i.e. only for databases this process instance previously observed as deviating. Walk through a restart landing between an alert and its recovery:

1. DB parks OFFLINE. Fires, persists last_alerted_state = 'OFFLINE'. active = {db}.
2. Service restarts (deploy/patch/crash) — active resets to {}. The persisted value is untouched (that's supposed to be the point).
3. Before the next eval cycle observes the database as still-deviating, it recovers to ONLINE.
4. First post-restart eval: the deviations query no longer returns this db (back to expected), so it's never in current — and active is empty, so it's never in recovered either. This line never runs. last_alerted_state stays 'OFFLINE' forever.
5. Weeks later the DB is parked OFFLINE again — the exact repeat-park workflow [FEATURE] Database State alert: per-state notify mode - fire OFFLINE/RESTORING once instead of re-firing every cooldown #2166 exists for. LastAlertedState ('OFFLINE') still equals StateDesc ('OFFLINE'), alreadyAnnounced is true, and the second parking is silently swallowed. Nobody is ever told.

This is narrower than the bug the 3rd commit fixed (needs a restart in the gap between alert and recovery, not just any recovery), but it's the same failure mode and it undercuts the PR's stated restart-survival goal. It's also a departure from how every other _active* flag in this file is documented to behave (lines 87-89: "In-memory only... the restart protection is the persisted watermarks... not these"). For those alerts, losing active on restart risks only a missed/duplicate resolution notification, because the persisted watermark that actually gates re-firing is a monotonic count/time that naturally supersedes stale values. Here the persisted value is a sticky state string whose only path to being cleared is this ephemeral set, so that invariant no longer holds for this alert.

DatabaseState_RepeatEpisode_OfTheSameState_FiresAgainAfterRecovery doesn't catch this because it drives the whole episode (alert → recover → re-park) through one AlertEngine/active instance — it never simulates a restart landing between the alert and the recovery.

A fix probably needs the clear to not depend on process-local state — e.g. have the adapter/store also identify databases with a non-null last_alerted_state whose current effective state now equals expected_state, so the memory can be cleared from what the store actually holds rather than from what this process happened to observe.

[claude]

Review

Went through the three commits (edge-trigger the chosen database states, fix the migration-doc reorder, and the author's own self-caught fixes for baseline-poisoning + falling-edge reset). Overall the design is sound and well-tested for the scenarios it targets — the UPDATE-only fix for SaveDatabaseStateAlertedAsync (never baseline a SUSPECT database via the alert-stamp write) and the falling-edge ClearDatabaseStateAlertedAsync are both real, necessary corrections.

One correctness gap remains (left as an inline comment on AlertEngine.cs:1337): the falling-edge clear that's supposed to make the new persisted last_alerted_state restart-safe is only reachable through _activeDatabaseStateAlerts, which is in-memory and resets on every process restart. If a service restart lands between an alert firing and that database's recovery, the recovery is never observed by the new process's active set, so ClearDatabaseStateAlertedAsync never runs and the persisted state goes stale forever — silently swallowing the next repeat-park episode, which is the exact workflow this PR exists to support. It's narrower than the bug the 3rd commit just fixed, but it's the same failure mode and none of the new tests exercise a restart between alert and recovery (DatabaseState_RepeatEpisode_OfTheSameState_FiresAgainAfterRecovery drives the whole episode through one engine instance).

Other notes, none blocking:

• Lite/Darling parity is handled correctly and honestly — Lite's SaveDatabaseStateAlertedAsync/ClearDatabaseStateAlertedAsync are documented no-ops rather than a fake in-memory cache, and the engine change is inert for Lite by construction (empty adapter read → every deviation reads as new, matching pre-[FEATURE] Database State alert: per-state notify mode - fire OFFLINE/RESTORING once instead of re-firing every cooldown #2166 behavior). No drift.
• SQL is all parameterized ($1/$2/$3 via Npgsql), no injection surface. Migration V60 is additive/nullable and idempotent (ADD COLUMN IF NOT EXISTS), consistent with the rest of the ladder.
• RepeatsAreNoise's XML doc only narrates OFFLINE/RESTORING in prose but the switch correctly also covers RECOVERING/STANDBY per the PR description — just a minor doc-vs-code completeness nit, not a behavior issue.
• No missing-index DMV suggestions here — not applicable to this change.

[erikdarlingdata]

Confirmed and fixed in 5be13e7. Third real one, and the diagnosis of why was the valuable part.

You're right that this alert can't follow the convention documented at lines 87-89. For the other _active* flags, losing the set on restart risks only a missed or duplicate resolution notice, because the persisted thing that gates re-firing is a monotonic count or time — a stale value is naturally superseded by the next observation. Here the persisted thing is a sticky state string whose only path to being cleared was that ephemeral set, so the invariant didn't carry over and I shouldn't have assumed it did.

Fixed by taking your suggestion literally: the clear is now derived from the store. ClearRecoveredDatabaseStateAlertsSql runs beside the existing SeedDatabaseStateExpectedSql and PruneDatabaseStateExpectedSql, on the same connection, and clears the memory for any database the store now shows back at its expected state. It can't have the gap because it asks rather than remembers, and it heals a stale memory regardless of how it got stale — restart, crash, or somebody editing the table by hand.

I kept the engine-side clear as well, and want to be explicit that this is two layers rather than accidental duplication: the engine clear is the immediate path, so a recovery inside one process doesn't wait for the next cycle's sweep, and the store sweep is what owns the invariant. If they ever disagree, the store wins by construction.

Three details that took some thought:

• It reads the same effective state the deviation query does — CASE WHEN is_in_standby THEN 'STANDBY' ELSE state_desc END — so a log-shipping secondary expected at STANDBY actually clears. Comparing against raw state_desc would have left exactly those rows stuck, which is a nasty variant of the same bug.
• It clears the (ignore) sentinel too. An operator silencing a database shouldn't leave a memory behind that outlives the silence.
• One sample at expected is enough, where the deviation rule needs two. Clearing is the safe direction — it can only cause an extra alert, never a missed one — and a flap can't exploit it, because a flap doesn't survive the two-sample deviation test to alert in the first place.

On testing: you were right that the harness test can't reach this, and I don't think any harness test can — the restart gap is defined by there being no process state, so anything driving the engine proves only the path a running process takes. So it's a gated-live test (DatabaseStateAlertMemoryLiveTests) that seeds five databases — recovered, still-deviating, deviating-differently, standby-effective-state, and (ignore) — runs the real statement against a real store with nothing in memory, and asserts a second sweep changes zero rows, since this now runs on every evaluation of every server.

Worth noting these three statements had no test coverage at all before this, so that's a small net improvement beyond the fix.

[claude]

Worth double-checking the "composition property" this PR pins (OFFLINE → SUSPECT must fire again at Critical): alreadyAnnounced correctly goes false on a state change, but the fire is still gated by CooldownElapsed(_lastDatabaseStateAlert, cooldownKey, now, alertCooldown), and cooldownKey (DatabaseStateCooldownKey) is keyed per database, not per state.

So if a database goes OFFLINE (alerts, stamps _lastDatabaseStateAlert[cooldownKey] = now) and then flips to SUSPECT within the same cooldown window, the transition-to-SUSPECT alert will be silently suppressed by the still-running cooldown from the OFFLINE alert — even though alreadyAnnounced is false for SUSPECT. That's exactly the "going quiet must not mean going blind" scenario the PR calls out as the important safety property, and none of the new tests exercise it because they all start from a fresh harness where the cooldown key has never been stamped (so CooldownElapsed trivially returns true).

This isn't introduced by this PR (the cooldown key was already shared across states pre-#2166), but the new edge-trigger feature raises the stakes: previously every deviation re-fired every cooldown regardless of state, so a same-cooldown-window SUSPECT would eventually re-announce on the next tick anyway. Now that OFFLINE goes quiet indefinitely, a fast OFFLINE→SUSPECT flip landing inside one cooldown window means the SUSPECT transition can be missed for a full cooldown period. Given SUSPECT is the integrity/safety-net case the PR explicitly designed to never go quiet, might be worth confirming this is acceptable (cooldown windows are typically much shorter than an outage) or keying the cooldown/announced-state check by state as well as database.

[claude]

Reviewed the diff (C#/Postgres, no T-SQL touched here). Overall this is solid — the design rationale in the commits/PR body is clear, and it's evident the branch already went through a few rounds of self-review (the UPSERT→UPDATE-only fix and the store-derived clear are both good catches on prior iterations).

Things I verified and didn't find issues with:

• Lite/Darling parity: intentionally Darling-only, and the "inert by construction" claim checks out — LiteAlertStateStore's new methods are no-ops, DatabaseStateInfo.LastAlertedState is never populated by LocalDataService.GetDatabaseStateDeviationsAsync (Lite's SQL doesn't select it), so RepeatsAreNoise(...) && LastAlertedState == StateDesc is always false there and behavior is unchanged. Both IAlertStateStore implementers (Lite, Darling, and every test stub) got the two new interface methods, so nothing is left unimplemented.
• V60 migration/version ladder: PgMigrations, StorageVersion, ViewerDataService.MapProbedSchemaVersion (new arg appended last with false default, correctly wired to the 43rd probe column), and all the ladder-tip test pins move together consistently.
• UPDATE-only stamp: confirmed SaveDatabaseStateAlertedAsync only ever UPDATEs (no INSERT/UPSERT path), matching the reasoning about not baselining a database first-observed in an integrity state.
• Falling-edge clear: the store-derived sweep (ClearRecoveredDatabaseStateAlertsSql) correctly reads the same effective-state CASE expression as the deviation query (standby-aware), clears the (ignore) sentinel, and runs before the deviation read each cycle so restart gaps can't leave a sticky memory — this is the right fix for the restart-window bug called out in the last commit.

One correctness edge case worth a look, left as an inline comment on AlertEngine.cs: the new alreadyAnnounced short-circuit is per-database-and-state, but the fire is still gated by the existing per-database (not per-state) cooldown key. A database that flips from an edge-suppressed state (e.g. OFFLINE) straight into an integrity state (e.g. SUSPECT) within the same cooldown window as the OFFLINE alert would have its SUSPECT transition silently swallowed by the still-running cooldown, even though alreadyAnnounced correctly evaluates to false for SUSPECT. None of the new tests catch this because they all start from a cooldown key that's never been stamped. Not something this PR introduces, but the new quiet-until-transition behavior raises the cost of that pre-existing gap for exactly the safety-critical case (integrity states) the PR is careful to protect elsewhere.

Minor/non-blocking style nit: PgAlertStateStore.cs and LiteAlertStateStore.cs both drop the blank line between the previous method's closing brace and the new method's doc comment, and leave a double blank line after ClearDatabaseStateAlertedAsync/SaveDatabaseStateAlertedAsync. Cosmetic only.

No security or missing-index concerns; the new SQL is fully parameterized and scoped by server_id.

[erikdarlingdata]

Fixed in 91a3ecf rather than accepted, because I think accepting it would contradict the property this PR claims as its reason for existing.

Your framing is exactly right: the shared cooldown key predates this PR, and this PR is what turns it from a delay into a silence. Before, every deviation re-fired every cooldown, so a transition suppressed by the previous state's clock re-announced on the next tick. Now that a chosen state goes quiet indefinitely there's nothing to retry it — the suppression lasts the full cooldown window — and the state it lands on is SUSPECT, which the PR explicitly designs to never go quiet. "Cooldown windows are typically much shorter than an outage" is true and still not a good enough answer for the integrity case.

So each state now carries its own clock, per your second suggestion. One consequence worth naming: recovery has to clear every state's clock for that database, not one, or a stamp left behind would rate-limit a later episode against a cooldown that started before the recovery — the same class of stale-state bug as the previous two findings, one layer down.

Two tests, because the fix can fail in both directions:

• DatabaseState_TransitionToADifferentState_IsNotSuppressedByThePriorStatesCooldown — both evaluations run back to back so no wall-clock time passes, which is precisely the condition that broke it. OFFLINE fires and stamps; the flip to SUSPECT must still fire, at Critical.
• DatabaseState_SameState_StillRateLimitsItself_WithinOneCooldown — proves the cooldown was re-keyed and not disabled. An integrity state repeats deliberately, so its own cooldown is the only thing between it and an alert per evaluation.

You were right that none of the existing tests could have caught this: they all start from a fresh harness where the key was never stamped, so CooldownElapsed returns true trivially. Same structural blind spot you identified on the state store — the tests never crossed the boundary where the state actually lives. That's four findings on this PR where the diagnosis of why the tests missed it was more useful than the bug report.

[claude]

Prefix-match delimiter collision: cooldownPrefix = key + "|" + dbName + "|", matched with StartsWith. A database literally named Foo|Bar produces a cooldown key key|Foo|Bar|<state>, which StartsWith("key|Foo|") also matches — so recovering Foo wipes Foo|Bar's cooldown clock too (and vice versa isn't possible the other way, but the false-positive prefix match is real). The doc comment above already calls this out and argues the blast radius is bounded ("at most one extra alert, never a missed one"), which is true, but it's a pre-existing gap that got wider here: the old code did an exact-key TryRemove (no collision possible), and this PR is what introduces the prefix sweep.

Since SQL Server does technically allow | in a (bracket-quoted) database name, this is reachable, not just theoretical. A cheap fix: delimit with a character that can't appear in a sysname (e.g. a control character, or length-prefix the segments) instead of |, or switch the cooldown store to a nested ConcurrentDictionary<string, ConcurrentDictionary<string, DateTime>> keyed by (serverKey|dbName) -> state -> timestamp so clearing a database's states doesn't need string matching at all.

[claude]

Review

Went through the engine change, the two new store methods, the V60 migration, the schema-probe wiring, and both live/harness test suites.

Correctness — the core logic holds up under scrutiny:

• The per-database-per-state cooldown key (AlertEngine.DatabaseStateCooldownKey) correctly prevents a state transition from being swallowed by the previous state's still-warm cooldown — verified against the TransitionToADifferentState_IsNotSuppressedByThePriorStatesCooldown test.
• alreadyAnnounced only short-circuits for RepeatsAreNoise states (OFFLINE/RESTORING/RECOVERING/STANDBY) and is compared against the effective state consistently on both the write side (SaveDatabaseStateAlertedAsync(..., db.StateDesc)) and read side (db.LastAlertedState vs db.StateDesc), so STANDBY-mapped databases aren't compared against a raw state_desc by mistake.
• The two independent clear paths — the adapter's store-derived ClearRecoveredDatabaseStateAlertsSql (survives restarts) and the engine's in-memory-active-set clear (immediate within one process) — are correctly redundant, not conflicting; the one-sample-clear-vs-two-sample-deviation asymmetry is sound (clearing can only produce an extra alert, never a missed one, since the deviation query is the sole gate on whether anything can fire at all).
• SaveDatabaseStateAlertedAsync being UPDATE-only (never upsert) is correctly load-bearing — pinned by DatabaseState_AlertedStamp_IsAnUpdate_NeverAnInsert, and the reasoning (an INSERT would baseline a SUSPECT database as its own expected state) is real, not defensive theater.

Left one inline comment on a delimiter-collision edge case in the new cooldown-prefix sweep (AlertEngine.cs) — low severity (bounded to "one extra alert"), already partially acknowledged in the PR's own doc comment, but worth a cheap fix since | is technically legal in a bracket-quoted SQL Server database name.

Lite/Darling parity — intentional and correctly scoped. LiteAlertStateStore's two new methods are documented no-ops rather than an in-memory cache, and the reasoning holds: with LastAlertedState always empty from LiteAlertReadAdapter, alreadyAnnounced is always false, so Lite's behavior is unchanged pre/post this PR. Worth noting the shared AlertEngine.cs cooldown-key fix (per-state instead of per-database) applies to Lite too, which incidentally also fixes Lite's pre-existing state-transition-swallowed-by-cooldown bug as a side effect — that's a parity improvement, not drift.

Security — no injection risk; all production SQL is parameterized. The one string-interpolated DELETE in the new live test (DatabaseStateAlertMemoryLiveTests.DeleteLiveRowsAsync) only interpolates a compile-time constant sentinel server_id, consistent with the existing live-test cleanup pattern elsewhere.

Minor nit (not filed inline, too trivial): both PgAlertStateStore.cs and LiteAlertStateStore.cs are missing a blank line between the preceding method's closing brace and the new method's /// doc comment, and have a stray double-blank-line before the trailing private helpers. Cosmetic only.

No missing-index or other DMV-folklore suggestions offered, per repo convention.

[erikdarlingdata]

Fixed properly in the latest commit rather than left documented, because you're right that this one is on me in a way the earlier findings weren't: the old code did an exact-key TryRemove, so no collision was possible, and my per-state re-key is what introduced the prefix sweep. "The blast radius is bounded" is a fine thing to say about an inherited gap and a weak thing to say about one I'd just created.

Your second suggestion is the one I took, slightly generalised. There's no delimiter a sysname cannot contain, so rather than hunt for a safer separator I stopped encoding a compound key as a string: _lastDatabaseStateAlert is now keyed by a (Server, Database, State) tuple, and recovery matches two of the three parts structurally. No string parsing anywhere in the path, so the bug class is gone instead of narrowed.

That needed CooldownElapsed to take a key type parameter — one line, every existing string-keyed caller infers unchanged, and the other nine families are untouched. I preferred that to giving this family a private copy of the cooldown helper, which is the kind of duplication that drifts.

The test pins your exact scenario: Foo and Foo|Bar both alerting, Foo recovers, Foo|Bar must stay quiet. Worth noting how it's observable, since it wasn't obvious — it has to run through an integrity state, because SUSPECT is the one case not edge-suppressed, so its cooldown is the only thing keeping it silent on the second pass and a wrongly-cleared clock makes it fire. Under the string-prefix version that test fails.

Five findings on this PR, and every one was real. The pattern I'll take away is that all five lived at a boundary the tests didn't cross — process memory vs store, one state vs another, one database's key vs another's.

[claude]

Nit: this justification doesn't hold for V60. It says "the viewer NAMES that table's columns in its expected-state editor, so the gate must refuse a V59 store rather than let a Settings read fail 42703" — but ViewerDataService.DatabaseStates.cs never references last_alerted_state anywhere (its seed/read/set/reset SQL only touches expected_state/is_user_override). Nothing in the Viewer would 42703 against a V59 store.

This looks like the V59 rung's comment (which is accurate — query_store_text_budget_mb really is named in ServiceConfigSelectSql/UpdateFlagsSql) got copy-pasted without updating for V60's actual situation. Bumping RequiredStoreSchemaVersion here is still correct (per the general RequiredStoreSchemaVersion == StorageVersion.SchemaVersion invariant this repo enforces every rung), just the stated reason is wrong and could mislead someone deciding later whether the gate can be relaxed for this column.

[claude]

Review summary

I read through the full diff (engine, both stores, migration V60, viewer schema gate, and the new tests) with a focus on correctness, Lite/Darling parity, security, and performance.

Overall: solid. The PR's own commit history shows five rounds of self-review that already caught and fixed real bugs (insert-vs-update baselining a SUSPECT database, the falling-edge clear depending on in-memory state across restarts, the per-database cooldown key colliding across states, and the |-delimiter collision in the cooldown key). I traced through the remaining logic — the store-derived ClearRecoveredDatabaseStateAlertsSql sweep, the engine's alreadyAnnounced/cooldown interaction, the tuple-keyed recovery cleanup, and the OFFLINE→SUSPECT→OFFLINE composition — and didn't find a live correctness bug in what's left.

Parity: Verified this is genuinely Lite-inert as claimed — LiteAlertReadAdapter/LocalDataService.DatabaseStates.cs never populates DatabaseStateInfo.LastAlertedState, so it defaults to "" and RepeatsAreNoise(...) && LastAlertedState == StateDesc can never be true for a real deviation. LiteAlertStateStore's new methods are documented no-ops. All four IAlertStateStore implementors (Pg store, Lite store, and both test stubs) implement the two new interface methods, so nothing is left half-wired.

Security: No new SQL injection surface — all new statements are parameterized ($1/$2/$3 via NpgsqlCommand.Parameters), including the new V60 migration and the live test's setup/teardown helpers.

One nit posted inline: the doc comment on the new V60/hasDatabaseStateEdgeMemory arm in ViewerDataService.MapProbedSchemaVersion claims the Viewer "NAMES that table's columns in its expected-state editor" to justify requiring a fully-migrated store — but ViewerDataService.DatabaseStates.cs never actually references last_alerted_state. Looks like the V59 rung's (accurate) justification got copy-pasted without adapting to V60's reality. The version bump itself is still correct per this repo's usual "Viewer requires the newest migration" convention — just the stated reason is wrong.

[erikdarlingdata]

Right on both counts — fixed in the latest commit.

The V60 justification was false, not just imprecise. I'd copy-pasted the V59 wording and it claimed the viewer names the new columns and would 42703 on a V59 store. It doesn't: the expected-state editor touches only expected_state and is_user_override, so nothing in the viewer reads either V60 column. Your read of how it got there is exactly what happened.

The bump is still correct, so the comment now gives the actual reason rather than being deleted: the standing RequiredStoreSchemaVersion == StorageVersion.SchemaVersion invariant, plus the point that it's the service — the alert engine's edge memory and the deviation query — that would fail against a V59 store, which is precisely why the viewer must not report V59 as good. Your last sentence is why I wrote it out at length instead of trimming it: someone deciding later whether this gate can be relaxed would have been reasoning from a false premise, which is worse than no comment.

The stacked summary was mine too — CooldownElapsed already had a one-line summary and I added a second above it instead of extending it. Merged.

That pin has now caught me three times in this change set, always the same way: writing a doc block for a member without checking whether it already had one. So rather than keep relying on CI to find it, I added a scripted stacked-summary scan over every file I touch as a pre-push step — it's what confirmed the other five files are clean this time. Worth noting the pin is doing real work; all three would have shipped as silently wrong documentation, since XML docs take the last block and tooling looks fine.

[claude]

Minor/low-severity: this recovery sweep now does a full scan of _lastDatabaseStateAlert.Keys — which holds entries for every server, database, and state currently tracked, not just this server — for each recovered database, filtering down with stamped.Server == key. Previously (string key) this was an O(1) TryRemove. It's now O(total tracked cooldown entries across all servers) per recovered database, run every evaluation cycle.

Given the entry count here is probably small (servers × databases × handful of chosen-states), this is unlikely to matter in practice, but flagging since it's a real algorithmic regression from the old per-database key. If it's ever worth avoiding, a secondary index keyed by (Server, Database) -> HashSet<State> (or just tracking the small set of states-in-play per active database) would make the sweep O(states-for-this-db) instead of O(everything).

[claude]

Review

Went through the full diff (engine, both stores, the V60 migration, viewer schema gate, and the new tests). Overall this is solid — the edge-trigger logic, the falling-edge reset, and the restart-survival story all hold up under scrutiny, and the test suite (including the delimiter-collision and repeat-episode cases) actually exercises the failure modes the PR description calls out. Specific things I checked and confirmed correct:

• Cooldown key migration to a tuple (Server, Database, State): verified every other CooldownElapsed call site still infers TKey=string correctly after the generic signature change — no breakage there.
• ClearRecoveredDatabaseStateAlertsSql ordering: it runs before the deviation SELECT on the same connection/transaction, so a cycle judges against a memory the store has already healed. Confirmed the single-sample clear vs. two-sample deviation-trigger asymmetry is safe (a flap can't exploit it, since the flap wouldn't survive the two-sample rule to alert in the first place).
• SaveDatabaseStateAlertedAsync UPDATE-never-INSERT: confirmed the no-row skip is safe — a row-less database was necessarily first observed in an integrity state (never edge-suppressed), so the memory this writes is never consulted for it. Matches the pinned AlertedStamp_IsAnUpdate_NeverAnInsert test.
• Schema-probe wiring: counted the StoreSchemaProbeSql SELECT list (43 columns) against MapProbedSchemaVersion's 43 bool params and the GetBoolean(0..42) call — all correctly appended at the end, no off-by-one.
• Lite parity: confirmed this is not accidental drift. DatabaseStateInfo.LastAlertedState defaults to "", LiteAlertReadAdapter/LocalDataService never populate it, and RepeatsAreNoise(...) && LastAlertedState == StateDesc is therefore always false for Lite — so Lite is provably unaffected by construction, exactly as the PR body claims, rather than by convention someone could accidentally break. All five IAlertStateStore implementers (prod + test stubs) got the two new interface methods.
• SQL injection / secrets: no string-built SQL from external input; all params go through AddWithValue. The one string-interpolated DELETE ... WHERE server_id = {id} is test-only cleanup code using a hardcoded negative sentinel constant, not user input.

Left one inline nit on the falling-edge recovery sweep (AlertEngine.cs) — a minor algorithmic regression (O(1) → O(total tracked entries) per recovered database) that's almost certainly fine at this system's scale, flagging for awareness rather than as a blocker.

No correctness bugs, security issues, or unintentional Lite/Darling parity drift found.

[erikdarlingdata]

Fixed — you're right that it's a real regression and not just a theoretical one, since the old string key made this an O(1) remove and the tuple key turned it into a scan of every server's entries, once per recovered database.

Hoisted to a single pass over the cooldown map against a HashSet of the recovered names, and skipped entirely when nothing recovered (which is the common case — recovery only fires when a database actually returns to its expected state). So it's O(tracked entries) per evaluation rather than O(recovered × tracked). Correctness is untouched: still per-state removal, since a single remove per database would leave its other states' stamps behind to rate-limit a later episode against a pre-recovery cooldown, and still matching on two tuple parts so Foo|Bar survives Foo recovering.

I did consider your secondary-index suggestion and decided against it, for what it's worth: (Server, Database) -> HashSet<State> would get it to O(states-for-this-db), but it's a second structure that has to stay in sync with the first, and it would need the same clearing discipline on every path that touches the primary map — which is precisely the class of "two places to remember" bug that produced three of the earlier findings on this PR. The factor I removed is the one that was actually new; the remaining single scan is the same order the surrounding recovery code already pays. If the entry count ever gets large enough to matter, the index is the right answer and this comment is the note explaining why it wasn't yet.

That should be all seven findings on this PR closed.

[claude]

recoveredSet is built with StringComparer.OrdinalIgnoreCase, but the comment right above current/active (lines 1233-1236) explicitly documents that per-database keys here must be Ordinal (case-sensitive) to match the store's case-sensitive database_name joins. recovered itself is sourced from that Ordinal active set, so this line silently widens the match.

Concretely: on an instance with a case-sensitive collation where databases Foo and foo coexist, recovering Foo will also TryRemove foo's per-state cooldown stamps from _lastDatabaseStateAlert (matched via recoveredSet.Contains(stamped.Database)), letting foo's cooldown — its only remaining quiet mechanism for an integrity state like SUSPECT — reset early and fire again ahead of schedule.

This is the same collision class the PR just finished eliminating for '|' in database names (structural tuple key instead of a delimited string) — worth using StringComparer.Ordinal here too for consistency with the rest of this method:

var recoveredSet = new HashSet<string>(recovered, StringComparer.Ordinal);

[claude]

Reviewed this PR (V60 edge-trigger for chosen database states, plus the five follow-up fixes already folded into the commit history).

Overall: this is a well-tested, carefully-reasoned change — the persisted-vs-in-memory tradeoff is argued convincingly, the tuple-keyed cooldown correctly avoids the '|'-in-database-name collision, the store-derived recovery sweep correctly closes the restart-gap, and the UPDATE-only stamp correctly avoids baselining a critical first observation. Lite parity is handled the right way: the interface grew two methods that Lite implements as documented no-ops, and since Lite's adapter never populates LastAlertedState (defaults to ""), the shared AlertEngine code is provably inert for Lite — verified against Lite/Services/LocalDataService.DatabaseStates.cs.

One finding posted inline: AlertEngine.cs:1352 builds recoveredSet with StringComparer.OrdinalIgnoreCase, which contradicts the Ordinal (case-sensitive) invariant documented three lines above it for the same per-database keys, and reintroduces — for case-differing database names — the same class of cross-database cooldown collision that this PR just finished eliminating for the '|' delimiter case. Narrow blast radius (only matters on a case-sensitive server collation with two databases differing only by case), but a straightforward one-word fix for consistency.

No other correctness, security, or Lite/Darling parity issues found. Migration V60 is additive/idempotent and the schema-version ladder (StorageVersion, MapProbedSchemaVersion, all the pinned ladder tests) is updated consistently.

[erikdarlingdata]

Correct, fixed. And this one stings a little, because it's the same collision class the tuple key had just eliminated for '|' — reintroduced one line later by a comparer choice.

Verified your reading against the code rather than taking it on faith: the comment at 1231-1235 does state per-database keys are ORDINAL to match the stores' case-sensitive expected-state joins, current is StringComparer.Ordinal, active is StringComparer.Ordinal, and my recoveredSet was the only outlier. On a case-sensitive collation with both Foo and foo, recovering Foo would have cleared foo's per-state stamps and reset the only quiet mechanism an integrity state has. Now Ordinal, with a comment saying why so the next person hoisting this doesn't widen it again.

While I was there I checked the other two OrdinalIgnoreCase uses in the method, since a blanket "make it all Ordinal" would have been wrong: the excluded-databases filter is deliberately case-insensitive (user-facing list, consistent with the other alerts) and the already-announced check compares state tokens, not database names. Both stay.

Eight findings on this PR now, every one real. The through-line is worth naming: each lived at a boundary — process memory vs store, one state vs another, one database's key vs another's, and now one casing of a database name vs another. The store-derived clear, the tuple key and this comparer are all the same lesson about identity having to mean exactly one thing on both sides of a boundary.

[erikdarlingdata]

Rebased onto dev after #2183 merged — the only real conflict was CHANGELOG.md, where both PRs added an entry under ### Changed; kept both, #2164's first since it landed first. Every other file was disjoint.

Rebased rather than merged so the nine commits stay linear and each review finding remains its own reviewable step.

Re-verified after the rebase rather than assuming it was mechanical: V60 is still the ladder tip (StorageVersion.SchemaVersion = 60, both V60Sql references intact), dev's QueryStorePlanXmlState.cs is present so the rebase didn't drop #2183's work, all seven affected projects build with zero errors (Collectors, Alerting, Darling.Service, Darling.Viewer, Darling.Tests, Lite, Lite.Tests), and the stacked-summary guard is clean across every file this branch changes.

[claude]

Muting a database-state alert can permanently silence it, even after the mute is removed.

SaveDatabaseStateAlertedAsync runs whenever this block fires, regardless of isMuted (line 1295) — muting only skips the deliverer's external channels (attemptChannels: !outcome.Muted in DarlingAlertDeliverer.SendAndRecordAsync), it doesn't stop the "announced" memory from being written.

For the four edge-triggered states (OFFLINE/RESTORING/RECOVERING/STANDBY), that memory is what gates all future firing until the state changes (alreadyAnnounced at line 1276). So the sequence:

1. Operator mutes the Database State alert for a database (or the metric globally) while it's OFFLINE.
2. Engine evaluates, sees the deviation, skips delivery (muted), but still calls SaveDatabaseStateAlertedAsync(key, dbName, "OFFLINE").
3. Operator removes the mute rule, expecting to be notified going forward.
4. Database stays OFFLINE. LastAlertedState == "OFFLINE" == StateDesc → alreadyAnnounced is true forever, so it never fires again — not even once — until the state actually changes.

This is a real behavior change from the old cooldown-repeat model: previously, unmuting would surface the alert again within one cooldown window regardless of what happened while muted. Now a database parked in one of these states can go permanently unnotified after a temporary mute, which seems like exactly the kind of silent gap this PR is trying to eliminate (just from a different cause). Worth deciding intentionally — e.g. only stamp last_alerted_state when !isMuted, or gate alreadyAnnounced on delivery rather than evaluation — and covering it with a test, since none of the new tests exercise mute interaction.

[claude]

Reviewed the #2166 edge-trigger change (engine, adapter SQL, migration V60, Lite no-op parity).

Left one inline finding on PerformanceMonitor.Alerting/AlertEngine.cs:1331: muting a database-state alert while a chosen state (OFFLINE/RESTORING/RECOVERING/STANDBY) is active writes the "announced" memory the same as an unmuted fire (mute only skips the deliverer's external channels), so alreadyAnnounced stays true after the mute is later removed — the alert can go permanently silent for that episode even though the operator never actually saw it. None of the new tests exercise mute interaction, so this seems like a genuine gap in an otherwise very thorough edge-case sweep, worth a decision either way.

Everything else checked out:

• The SQL in ClearRecoveredDatabaseStateAlertsSql and the deviation query use the same effective-state (STANDBY-aware) computation, and the live test (DatabaseStateAlertMemoryLiveTests) exercises all five recovery/keep/ignore/standby branches correctly.
• SaveDatabaseStateAlertedAsync is UPDATE-only (never upsert) — correctly avoids baselining a first-observed SUSPECT database as its own expected state, and the reflection-based test (DatabaseState_AlertedStamp_IsAnUpdate_NeverAnInsert) pins it.
• The tuple-keyed cooldown dictionary correctly fixes the Foo/Foo|Bar delimiter-collision bug class, and the recovery sweep's full-dictionary scan is a deliberate, documented trade-off for correctness over the old O(1) string-key removal — reasonable given the small state cardinality per database.
• V60 migration, StorageVersion, and the Viewer's MapProbedSchemaVersion positional-boolean gate are all consistent (new column appended last in the SQL, reader, and method signature).
• Lite parity is handled correctly: IAlertStateStore's two new methods are proper no-ops there (not a fake in-memory cache), documented as deliberately deferred, matching the #2167 → #2176 precedent — this is disclosed drift, not a hidden one.

Minor/non-blocking: both PgAlertStateStore.SaveDatabaseStateAlertedAsync and LiteAlertStateStore.SaveDatabaseStateAlertedAsync are missing the blank line before their new XML doc comments (immediately follow the previous method's closing brace), and both have a stray double-blank-line before their final private helper — inconsistent with the blank-line-between-members convention used elsewhere in both files.

[erikdarlingdata]

Real bug, fixed. And it's the same failure mode this PR exists to remove, reached from a direction I hadn't considered: an operator's temporary mute silently became irreversible.

You've also identified the one place where this memory and the cooldown next to it must disagree, which is worth stating in the code rather than leaving as a subtlety. The cooldown is rate limiting — it applies whether or not anyone was told, which is exactly why the line above it is stamped even when muted. This memory means "the operator has been told about this state," and under a mute they have not. So the stamp is now skipped when muted while the cooldown stamp stays.

I took your first option rather than gating alreadyAnnounced on delivery, because delivery success is a different question again — the existing comment notes the stamp is deliberately written after FireAsync so a failed fire retries next cycle rather than being silenced. Tying the memory to delivery outcome would entangle "nobody was told because they muted it" with "nobody was told because SMTP was down," and those want different handling. Muted is knowable up front; delivery failure isn't.

Cost of the fix is a repeat inside the mute, which is invisible by definition and is precisely the pre-#2166 cooldown behaviour for a muted alert.

The test drives the whole sequence instead of just asserting the stamp is absent, using the harness's injected clock: muted fire must be marked muted and leave the memory empty; then with the mute removed and the cooldown elapsed, the same still-deviating state must produce a real deliverable alert and stamp the memory. Under the old code that second half is silent, which is the regression.

You were right that no existing test touched mute interaction — that's now three findings on this PR whose root cause was a boundary the tests never crossed, so I'll take the hint about where to look first next time.

[claude]

Nit: missing blank line between SaveFailedJobWatermarkAsync's closing brace (line 164) and this new doc comment — every other member in this file is separated by one blank line. Same nit applies at the other end: lines 235-236 have two blank lines before NaiveUtcNow() instead of one.

[claude]

Nit: missing blank line before this doc comment (line 94 } runs straight into line 95), and there's a double blank line at 110-111 before ParseServerKey. Every other member in this file uses a single blank-line separator.

[claude]

Review summary

Traced the full engine/store/adapter change for the database-state edge trigger. Overall this is careful, well-reasoned work — calling out what I checked and why it holds up, plus two small nits.

Correctness

• The alreadyAnnounced gate (RepeatsAreNoise(db.StateDesc) && LastAlertedState == StateDesc) is only consulted for OFFLINE/RESTORING/RECOVERING/STANDBY; SUSPECT/RECOVERY_PENDING/EMERGENCY always fall through to the existing cooldown, matching the stated intent and covered by DatabaseState_IntegrityState_StillRepeats_EvenWhenAlreadyAnnounced.
• Verified the composition case explicitly: a database whose memory says OFFLINE but is now SUSPECT fires again at Critical (DatabaseState_ChosenState_ChangingToADifferentState_FiresAgain), and that a same-cooldown-window transition to a different state isn't swallowed by the old state's clock, now that the cooldown key is (server, database, state) rather than per-database — good regression coverage of the case that would otherwise be a real bug (DatabaseState_TransitionToADifferentState_IsNotSuppressedByThePriorStatesCooldown).
• Mute handling is correct and matches the doc comment's reasoning: the in-memory cooldown stamps even when muted (rate limiting), but the persisted last_alerted_state is deliberately not stamped when muted — otherwise unmuting a parked database would never re-fire, since the edge gate would see it as already-announced forever. DatabaseState_MutedFire_DoesNotRecordItAsAnnounced_SoUnmutingStillNotifies pins this.
• ClearRecoveredDatabaseStateAlertsSql (store-derived clear, runs every cycle before the deviation read) plus the engine's own falling-edge clear together close the restart-gap: a restart landing between an alert and the recovery no longer leaves last_alerted_state stuck forever, since the store-side clear doesn't depend on the engine's in-memory active set. The asymmetry (recovery clears on one sample, deviation requires two) is explicitly reasoned about and is provably safe in the direction that matters — worst case is one extra alert after a single-sample flap, never a missed one.
• SaveDatabaseStateAlertedAsync is UPDATE-only, never upsert/insert — correctly avoids the trap of baselining a SUSPECT database as its own expected state (which would silently stop it deviating and swallow future integrity alerts). Pinned by a source-inspection test rather than relying on runtime behavior alone.

Lite/Darling parity

• The shared AlertEngine.cs change is genuinely inert for Lite: LiteAlertStateStore's new methods are documented no-ops, so LastAlertedState always arrives empty from Lite's adapter and alreadyAnnounced is always false there — Lite keeps exactly its pre-[FEATURE] Database State alert: per-state notify mode - fire OFFLINE/RESTORING once instead of re-firing every cooldown #2166 cooldown-repeat behavior. This is called out as intentional in the PR description and changelog, and it's actually true in the code, not just asserted.
• All IAlertStateStore implementers (Darling's PgAlertStateStore, Lite's LiteAlertStateStore, and both test stub stores in AlertEngineTests/DarlingSelfAlertTests/LiteAlertForwardingTests) got the two new interface methods — no implementer left half-updated.

Security

• All new SQL is parameterized ($1/$2/$3 via NpgsqlCommand.Parameters.AddWithValue), no string-built predicates from external input. The one raw string-interpolated DELETE in the new live test (DatabaseStateAlertMemoryLiveTests.DeleteLiveRowsAsync) only interpolates a hardcoded negative sentinel constant, not external input, and matches the existing "house pattern" used by other live tests in this suite.

Minor nits (posted inline)

• PgAlertStateStore.cs and LiteAlertStateStore.cs: a couple of missing/doubled blank lines between members where the new methods were inserted — purely cosmetic, doesn't affect behavior.

No missing-index-DMV suggestions offered per instructions. Nothing here rises to a blocking finding.

[erikdarlingdata]

Both fixed — a doc comment running into the preceding closing brace, and a double blank before the private helper, in each of the two state stores. Real inconsistencies I introduced; every other member in both files uses a single blank-line separator.

Small process note since it nearly bit me: both files are CRLF, and my first pass anchored on bare LF and matched nothing. The assertion on anchor count caught that before anything was written rather than after, which is exactly why I anchor-assert scripted edits in this repo instead of trusting a replace to have done something. Verified afterwards that CRLF counts are unchanged (114 and 243) with zero bare LF introduced, and that no triple-newline runs remain in either file.

That's every review finding on this PR closed — nine substantive plus these two. Green across all five checks and CLEAN on merge state as of the last run; this one's ready pending whatever CI says about this push.

[claude]

Minor performance note: this sweep now does a full linear scan of _lastDatabaseStateAlert.Keys on every database recovery — and that dictionary holds cooldown entries for every monitored server, not just the one currently being evaluated, since it's a single field shared across the whole engine instance.

The comment above frames the tradeoff as "one full scan" vs. "O(recovered × everything tracked)", but there's a cheaper option that keeps the old O(1)-per-database behavior without reintroducing the string-prefix collision bug: DatabaseStateTokens only defines a small, fixed vocabulary of state tokens (OFFLINE, RESTORING, RECOVERING, STANDBY, SUSPECT, RECOVERY_PENDING, EMERGENCY). For each recovered database you could just TryRemove((key, dbName, state)) directly for each of those ~7 known tokens instead of enumerating the shared dictionary — O(recovered × 7) with no scan of other servers' entries at all.

Probably not a big deal at today's fleet sizes, but worth a look since this runs on every recovery event, for every server, against one shared dictionary.

[claude]

Review summary

Went through the whole diff (engine, adapter SQL, migration, both apps' stores, and the test suite). This is a carefully done change with unusually thorough test coverage for the edge cases that actually matter here (restart-survives-memory, mute-doesn't-permanently-silence, |-in-database-name key collision, falling-edge-clears-so-repeat-episodes-still-fire). I traced the logic end to end and didn't find correctness bugs:

• Effective-state consistency: DatabaseStateDeviationsSql, ClearRecoveredDatabaseStateAlertsSql, and the seed SQL all compute the STANDBY-mapped effective state the same way, so db.StateDesc/LastAlertedState comparisons in AlertEngine are apples-to-apples.
• Mute semantics: the persisted memory is correctly skipped on a muted fire (if (!isMuted) await _stateStore.SaveDatabaseStateAlertedAsync(...)), which is what keeps unmuting meaningful — verified against DatabaseState_MutedFire_DoesNotRecordItAsAnnounced_SoUnmutingStillNotifies.
• UPDATE-never-INSERT invariant: confirmed the save can't accidentally baseline a SUSPECT database as its own expected state (no row exists for never-baselined critical states, and those states are never edge-suppressed anyway, so the skipped write is never consulted).
• Interface parity: IAlertStateStore gained two members; every implementer (PgAlertStateStore, LiteAlertStateStore, and both test stub classes) was updated, so nothing is silently missing a case.

Lite/Darling parity: this is Darling-only by design, and it's genuinely inert for Lite rather than a drifted implementation — LiteAlertReadAdapter.GetDatabaseStatesAsync was untouched, so LastAlertedState is always "", which makes alreadyAnnounced always false and preserves Lite's exact pre-#2166 cooldown-repeat behavior. LiteAlertStateStore's new methods are documented no-ops rather than an in-memory cache, which is the right call (a cache would go quiet and then mass-re-fire on restart — worse than what it replaces). Called out clearly in the PR description and changelog, so flagging for visibility only, not as a defect.

One minor performance note left as an inline comment on AlertEngine.cs — the per-recovery cooldown-map cleanup now does a full scan of the shared cross-server dictionary instead of a bounded number of targeted removals over the small fixed state vocabulary. Not likely to matter until fleet size grows, but a cheap fix.

No security concerns — all new SQL is parameterized, no new external input paths.

[erikdarlingdata]

Considered and declining this one, with a specific reason rather than on cost grounds — and merging, since every substantive finding is closed and all five checks are green.

The enumerate-the-known-tokens approach would leak entries. DatabaseStateTokens defines ONLINE, OFFLINE, RESTORING, RECOVERING, RECOVERY_PENDING, SUSPECT, EMERGENCY and STANDBY — but the cooldown key is built from db.StateDesc verbatim, and sys.databases.state_desc also yields COPYING and OFFLINE_SECONDARY (Azure SQL geo-replication states) which that list does not contain. A database in one of those would create a cooldown key that removal-by-known-token could never find, leaving a stale stamp behind to rate-limit a later episode against a pre-recovery cooldown.

That is precisely the failure class three earlier findings on this PR shared: stale state on one side of a boundary silently suppressing a real alert. Trading a rare scan for a new instance of it isn't a good deal, and guarding it would mean keeping the token list exhaustive forever against a vocabulary Microsoft controls — a coupling I'd rather not create.

Worth noting the scan is also narrower than it reads: entries only exist for databases that have actually fired, so the dictionary holds deviating databases, not all servers × all databases. And it runs only on a recovery event, which is by definition rare — recoveries are the transition out of an alerting state, not a per-cycle occurrence.

If entry volume ever becomes real, the right fix is the secondary index from your earlier comment — (Server, Database) -> HashSet<State> — which is exhaustive by construction rather than by maintained list. This thread is the note explaining why neither is in yet.

Ten findings on this PR, all ten legitimate. Six were genuine bugs (the SUSPECT baseline, the permanent memory, the restart-gap clear, the cooldown-borrowed-across-states silence, the delimiter collision, the permanent mute), and the diagnosis of why the tests missed them was consistently more valuable than the report itself. Thank you.