PagerDuty: de-duplicate edge-type alerts into one incident

[ianwalkeruk]

What does this PR do?

Adds two related improvements to the PagerDuty channel, shared by Lite and Darling through PerformanceMonitor.Notifications:

1. Edge-type alerts de-duplicate into one PagerDuty incident — when the operator opts in. The edge pairs —
   a monitored connection's Server Unreachable / Server Restored, an availability group's
   AG Replica Disconnected / AG Replica Reconnected — are two halves of ONE incident, but PagerDuty showed two: when an alert carries no incident fingerprint the dedup_key fell back to the metric name, and the metric name encodes the current state, so each edge minted its own key. With the new setting on, DerivePagerDutyDedupKey renames both edges of a pair to the pair's canonical firing name via AlertFamily.RecoveryPairs, so both edges share {serverId}:Server Unreachable (and {serverId}:AG Replica Disconnected:{AG}:{replica}, scoped per replica — one replica's reconnect must close only ITS incident), and PagerDuty groups the pair into one incident. The rename deliberately runs before the fingerprint path so a closing edge can never re-split the pair, and any pair added to RecoveryPairs later joins by construction. With the flag on, the helper's three consumers — the PagerDuty payload, the generic {{dedup_key}} token, and the triage link — all share the normalized key for a paired edge; with it off (the default), every consumer keeps the shipped per-state keys byte-identical — the closing edge does NOT correlate into the firing edge's incident, and correlation is a behaviour change scoped entirely to the operator who opted in. The AG pair's per-replica scope rides AlertContext.AgReplicaIdentity, set by both engines' AG fire sites; a pair that arrives without the identity keeps the per-server key rather than failing the render.
2. An opt-in "Auto-resolve the incident when the alert's condition clears" setting. With it on, the closing
   edge is sent as Events v2 event_action: "resolve" on the same dedup_key, closing the incident the firing
   edge's trigger opened — and a delivered close re-arms the pair's firing metric in the shared cooldown, so the
   next down (or next disconnect) actually posts instead of meeting the old incident's window. Off (the default),
   the closing edge remains an info-severity trigger on its own state-named key — nothing in the default path
   correlates across edges, and nothing is auto-resolved unasked. Test sends always trigger, firing edges never
   resolve, and Teams/Slack/generic/email are untouched.

Round-2 review fixes carried in this update, all pinned by tests:

• The delivered-close clear survives the history seed. ClearMetric removed the in-memory key, but
  IncidentCooldown.EvaluateAsync re-seeded a missing key from the alert history — and both production paths pass a
  history store — so the next "Server Unreachable" after a delivered restore was throttled against the FIRST
  outage's own send row. The clear now records its instant per metric-level key; the seed ignores any row older
  than it (a genuinely post-clear send still re-arms normally), and the record itself evicts under the same
  2×-window rule as the cooldown entries.
• The per-replica identity survives the render copies. IncidentDeliveryFilter's filtered copy and roster
  press, and PerEventNotification's per-message contexts, rebuilt their AlertContext from selected members and
  dropped AgReplicaIdentity — so in Summary mode a repeat that went through the filter derived the PagerDuty key
  from a copy that had lost the replica, and the resolve missed its per-replica incident. All three copy sites
  carry the identity now (the reflection census in Lite.Tests pins AlertContext's member set so the next member
  added is a decision about the copies).
• Accepted behaviour, documented: the delivered-close clear is per server + pair (the metric-level key), so one
  replica's reconnect also re-arms a sibling replica still down on the same server — bounded by its per-replica
  PagerDuty incident remaining open, and only ever consulted by a new firing.
• The always-true EXISTS assert Erik flagged in QueryStatsHourLedgerTests is gone (removed with the
  webhook-secrets merge), and the PR text here was corrected: default-off correlation claims removed, the shared-key
  claim made opt-in-conditional, V165 rebuilt as V166, and the test inventory refreshed.

Plumbing:

• IAlertSettings.PagerDutyAutoResolve is a defaulted interface member (false) — the deprecated Dashboard
  and every test fake keep the shipped behaviour with no code change, per the UncorroboratedFindingRoute /
  NotificationRoutes DIM precedent.
• Lite: setting persisted as pagerduty_auto_resolve in settings.json (documented in
  settings.sample.json), authored by a checkbox in the Settings window beside the EU-region toggle.
• Darling: migration V166 (pagerduty-auto-resolve, the ladder's next free version above the password-key
  rung's 165 top) adds config_notification.pagerduty_auto_resolve boolean NOT NULL DEFAULT FALSE — existing rows
  keep the shipped behaviour; StorageVersion.SchemaVersion = 166. Carried by the service's notification read
  (column appended last, so no existing ordinal re-maps), the Viewer's full + secret-free selects and upsert ($20),
  the schema probe's newest COLUMN-existence sentinel (hasPagerDutyAutoResolve, probe ordinal 141) and map arm (a
  fully migrated store maps to exactly SchemaVersion), and the Viewer's Settings window. The column is a behaviour
  toggle, not a credential, so — unlike the routing key beside it — it stays in the viewer/mcp roles' SELECT
  grants (DarlingManagedRoles.ViewerRestrictedConfigTables + tools/provision-roles.sql).

Tests: PagerDutyAutoResolveRungTests pins the V166 rung, its DDL shape, the probe's three sites, every
notification-row surface, the ACL classification and the settings seam, with the handoff from QueryStatsHourLedger's
pin in QueryStatsHourLedgerTests (the established pattern). The send path is pinned THROUGH
WebhookAlertService.TrySendWebhookAlertsAsync — three send-path tests in Lite (PagerDutyWebhookTests: the
resolve gated on the flag; the replica-scoped reconnect resolve; the down-back-down re-arm through a seeding
history-store fake, so the delivered-close clear is proven against the re-seed that was undoing it) and two
wire-level paired-lifecycle tests in Darling (AlertDeliveryChannelTests, flag on and off). The builder-level pins
remain in Lite (PagerDutyWebhookTests) covering the shared per-pair key (connection and replication alike), the
default info-level trigger, and the opt-in resolve. The delivery-filter's copy sites carry their own pins: the
identity survives the roster press, the filtered render and the derived per-replica key
(Darling.Tests.IncidentDeliveryFilterTests), the per-event split keeps it (PerEventNotificationTests), and the
AlertContext member census + the copy-carry assert hold every future member honest (AlertIncidentRenderTests).
Lite's cooldown-side pins (IncidentCooldownTests) cover the clear surviving a seeding history store in BOTH
directions — a pre-clear row stays ignored, a post-clear send still re-arms — plus the evicted-clear bound.

Which component(s) does this affect?

• Lite
• Darling
• Lite Tests
• Darling Tests
• SQL collection scripts
• Documentation
• Full Dashboard (deprecated)
• CLI Installer (deprecated)

How was this tested?

• SQL Server version(s): N/A — no SQL Server-facing code changed. No collector T-SQL, install/ script, or
  SQL Server read/write is touched; the channels consume alert outcomes the collectors already produce, and the
  only DDL is the Darling store's PostgreSQL migration V166.

• PostgreSQL: V166's DDL is pinned textually (schema-qualified, IF NOT EXISTS, non-null FALSE default, no
  DML/grant/trigger) and the ladder/probe census pins assert density and the 166 top. The env-gated live store
  tests (DARLING_TEST_PG / DARLING_TEST_PGRUNTIME) were not run locally and will exercise the migration in
  gated CI.

• Builds: dotnet build -c Release (per project, matching CI order): Notifications, Darling Storage, Service,
  Viewer, Lite, Dashboard, Installer, plus both test projects — 0 warnings / 0 errors each (analyzer-equal
  to Debug; Debug differs only by optimization). dotnet build -c Debug spot-checked.

• Suites: the full ungated suites were run on this box — Darling.Tests as a single host (27,522 tests,
  2,616 skipped: the env-gated live tests, which is the no-DARLING_TEST_PG/DARLING_TEST_SQL expectation) and
  Lite.Tests as sequential class chunks covering every class exactly once (8,827 tests; the box parks a
  single all-class host, so the union stands in for one host — every summary is a per-chunk execution). Red set:
  in Darling, the 8 machine-speed budget/hang-guard pins in the statement-filter suites and the culture-dependent
  browser-date pin in WebQueryStoreGridTests; in Lite, the machine-time-zone pin in HistoryGridZoneTests — the
  same pre-existing environmental set that fails identically on the upstream baseline, none touching the
  PagerDuty/cooldown/delivery-filter surfaces this change edits.

• Targeted verification steps:

  • With the flag on: Server Restored and Server Unreachable derive the same dedup_key; likewise the AG
    replica pair per replica ({serverId}:AG Replica Disconnected:{AG}:{replica}); a non-edge alert still keys on
    {serverId}:{metricName}.
  • Default (flag off): closing edge → event_action: "trigger", severity info, on its OWN state-named key;
    incident stays open.
  • Opt-in: closing edge → event_action: "resolve" on the firing edge's key; AG Replica Reconnected closes only
    ITS replica's incident; firing edges, high-CPU, and test sends still trigger even with the flag on.
  • Down-back-down on a seeded history store: three posts — the delivered close's clear wins over the pre-clear
    history row.
  • The Viewer probe maps a fully-migrated store to exactly SchemaVersion (166), one-rung-short stores to 165,
    etc. — the connect-time gate can no longer refuse a current store.

Checklist

• I have read the contributing guide
• My code builds with zero warnings (dotnet build -c Debug) (verified on Release per project — analyzer configuration is identical; Debug differs only by optimization)
• I have tested my changes against at least one SQL Server version (N/A — no SQL Server-facing code or T-SQL changed; see above)
• I have not introduced any hardcoded credentials or server names (the routing key remains a Credential Manager / store-column secret; nothing new carries a credential)

[erikdarlingdata]

Claude posting for Erik Darling.

Thank you for this PR. The migration, the role grants, and the Lite and Darling settings look right. Four things need to change before it merges.

1. One replica that reconnects can resolve an incident that another replica still has open. The key at WebhookAlertService.cs:2665 is {serverId}:AG Replica Disconnected, with no availability group or replica name. Lite and Darling raise these alerts per replica but pass no context. So every replica on a server shares one key. If auto-resolve is on and replicas A and B disconnect, A reconnecting closes the incident while B is still down. The re-fire setting defaults to 0, so nothing opens it again. Please add the availability group and replica name to both keys of the pair. Or leave the AG pair out of auto-resolve.

2. A server that goes down, comes back, and goes down again leaves no open incident. The recovery resolves the incident. The second "Server Unreachable" then meets the cooldown at WebhookAlertService.cs:228. That cooldown still uses the original metric name (IncidentCooldown.cs:131), so the alert is not sent. One fix: when auto-resolve is on, clear the cooldown entry for the paired alert when the recovery is sent.

3. The key change also applies when the setting is off. The rename at WebhookAlertService.cs:2665 does not check the setting. With the default, "Server Restored" now uses the "Server Unreachable" key. That changes {{dedup_key}} for generic webhook users and the triage link, and IAlertSettings.cs:98-102 says the default keeps the shipped behavior. There is also a risk. Someone can resolve the outage incident before the server comes back. Then the recovery can open a new info alert under the outage key. The next outage can merge into that alert instead of paging. Please rename the key only when PagerDutyAutoResolve is true.

4. No test covers the send path. The new tests call BuildPagerDutyPayload directly. They still pass with autoResolve: _settings.PagerDutyAutoResolve at WebhookAlertService.cs:2413 removed. Please add one test that sends through the service with an IAlertSettings that returns true, and checks the payload.

Two small items:

• ViewerDataService.Notification.cs:273-278: the last lines of Equals and its closing brace lost their indentation.
• QueryStatsHourLedgerTests.cs:192: before.LastIndexOf("EXISTS") >= 0 is always true when the line above it passes, so you can remove it.

You do not need a changelog entry, because we add it when the PR merges. The failed check-version job ran while the PR targeted main, and you can ignore it.

[erikdarlingdata]

Claude posting for Erik Darling.

Thanks for the quick turnaround on round 1. Four of the six points are fixed, and two are only part-way there. The re-check also found two new problems. Your branch had fallen behind our dev and had merge conflicts, so I fixed them: I merged dev into your branch as a990a0678, a plain merge commit with no rebase. The details are under "Already done in the merge" below. Please pull before you push again. If your branch falls behind dev again while you work on this, just say so here and I'll do the merge again, including renumbering the schema rung if another one lands first.

Round 1, point by point

#	Point	Status
1	The AG key had no AG or replica in it	Fixed. The key now carries the group and replica name, and it's set on both edges in Lite and Darling.
2	Down, back, down: the second "Server Unreachable" hit the old cooldown	Part-way. See below.
3	The key rename ran with the setting off	Fixed. The shipped key is kept unless auto-resolve is on.
4	No test covered the send path	Fixed, with three send-path tests and two delivery-channel tests.
5	Indentation in ViewerDataService.Notification.cs	Fixed.
6	The always-true assert in QueryStatsHourLedgerTests	Part-way. Assert.Contains("EXISTS", before) is still always true, because the arm text sits inside EXISTS (SELECT 1 WHERE to_regclass(.... Please just remove the line.

Still to do

1. The cooldown clear doesn't survive the history seed (point 2).
   • The problem: ClearMetric removes the in-memory key, but IncidentCooldown.EvaluateAsync re-seeds a missing key from the alert history (seedLastSentUtc, lines 75–79). Both production paths pass a history store: the Darling worker and Lite's main window. So the next "Server Unreachable" reads the first outage's send time back from history, and it's throttled again. The tests pass only because none of them uses a seeding history store.
   • The fix: make the clear stick, for example a "cleared at" time per key that the seed respects, so a history row older than the clear is ignored. Add a send-path test with a fake history store that returns the first outage's send time.
2. The replica identity is lost when the alert is copied for rendering.
   • The problem: IncidentDeliveryFilter builds new AlertContext objects at about lines 169 and 236, and copies only the severity, attachment and incidents. PerEventNotification.NewContext does the same. The PagerDuty key is derived from that copy (renderContext, WebhookAlertService.cs about line 322). In Summary mode, a repeat that goes through the filter therefore falls back to the per-server key, and the resolve misses the per-replica incident.
   • The fix: copy AgReplicaIdentity in those places, or derive the key from the original context, and add a Summary-mode test. (I traced this by reading the code. I didn't run it.)
3. Minor: clearing on an AG reconnect is per server, so one replica coming back also re-arms a sibling replica that's still down. A comment saying that's accepted is fine.
4. The PR description is out of date:
   • it says the closing edge correlates into the open incident by default, which is no longer true;
   • it says all three consumers share the normalised key, which is no longer true for the generic webhook;
   • the test count has changed;
   • it names V165.

Already done in the merge

• dev took schema V165 (the password-key tables) first, so your rung is now V166, with its body unchanged.
  • The probe sentinel is ordinal 141, with its own hasPagerDutyAutoResolve arm above V165's.
  • PagerDutyAutoResolveRungTests is re-pinned.
  • The role comments and provision-roles.sql say V166.
• A conflict-free file did conflict in meaning: dev's webhook-secrets change dropped PagerDutyAutoResolve from the opened settings copy, so the setting was always off. Your own test caught that, and the merge fixes it.
• Security is clean. The test routing keys are fakes, the send override is internal and never set in production, and the new column is non-secret and granted correctly.

[ianwalkeruk]

Round-2 items, all in one review-fix commit on top of a990a0678:

Point 6 (always-true assert) — already gone: dev's webhook-secrets merge (aa649b448) removed the EXISTS-bound assert when it re-pinned the probe for the new rung's ordinal; nothing left to delete, and the rest of the method's pins are unchanged.

Point 2 part-way (the clear must survive the history seed) — done in IncidentCooldown: ClearMetric now records the clear instant per metric-level key, and the seed branch in EvaluateAsync ignores any history row older than it, so the delivered close's clear keeps winning over the pre-clear row both production paths would otherwise have re-seeded. A locally-recorded post-clear send still re-arms normally (a genuinely new send after the resolve applies as before), and the clear record itself is evicted under the same 2×-window rule so both maps stay bounded. Tests: Lite.Tests.IncidentCooldownTests.ClearMetric_BeatsAPreClearHistoryRow_AndAPostClearSeedStillApplies (both directions, mutable seed fake), and the send path now runs its down-back-down three-post pin with a seeding history store (PagerDutyWebhookTests.SendPath_WebhookCooldown_AutoResolveRecovery_RearmsItsFiring) — previously the send-path test only worked on a store-less cooldown, which is why the regression hid.

New finding 1 (replica identity lost in the render copies) — IncidentDeliveryFilter's filtered copy and WithRoster, and PerEventNotification.NewContext, now carry AgReplicaIdentity, so a Summary-mode repeat deriving the PagerDuty key from the rendered copy keys per replica as built. WithRoster's remarks now enumerate five members. Reproduced first: three pins in Darling.Tests.IncidentDeliveryFilterTests (roster-press copy, filtered render, and the derived replica-scoped key through a folded render) are red when the identity copy is removed and green with it; the per-event split carries it too (PerEventNotificationTests). The census pin Erik's remark cites didn't exist yet — AlertIncidentRenderTests.TheDeliveryFilterCopies_CarryEveryRenderMember_TheCensusPinsAlertContext (Lite.Tests) now pins AlertContext's full public member set exactly, plus the five render members surviving both copy sites, so the next member added to the type is a decision rather than an accident.

New finding 2 (per-server clear re-arms a still-down sibling) — documented as accepted at both the fan-out's clear site and ClearMetric: the clear is per server + pair (the metric-level key), so one replica's reconnect also re-arms a sibling still down on the same server; the sibling's own incident stays open in PagerDuty under its per-replica key, and the re-armed entry is only consulted by a new firing.

PR description — refreshed: the default-off correlation claim is gone (each edge keeps its own state-named key), the shared-key claim is now opt-in-conditional (PagerDuty payload, generic {{dedup_key}} and the triage link agree only with the flag on), the test inventory matches what ships, and the rung is stated as V166 everywhere.

Validation: per-project Release builds (Notifications, Darling Storage/Service/Viewer, Lite, Dashboard, Installer + both test projects) at 0 warnings, Debug spot-check clean; full ungated suites run on this box (Darling in one host; Lite in sequential class chunks covering every class once — the box parks a single all-class host, so the run is the union of per-chunk executions; counts in the PR body) — the only reds are the same pre-existing environmental set that fails identically on the upstream baseline (the machine-speed budget/hang-guard family in the statement filter suites, the culture-dependent browser-date pin, and the machine-time-zone Lite grid pin), none touching the PagerDuty/cooldown/filter surfaces.

If dev moved meanwhile, say the word and I'll ask you to re-merge as before rather than merging here.

[erikdarlingdata]

Claude posting for Erik Darling.

Round 2 is done. The cooldown clear now holds after the history seed, the replica identity survives the delivery and per-event copies, the sibling re-arm is documented, the always-true assert is gone, and the description is current. The full CI run is green. Thanks for working through all of this. Merging now.