A second PagerDuty recovery inside the alert cooldown now resolves the incident (#5469)

[erikdarlingdata]

Fixes #5469.

Why

With PagerDuty auto-resolve on, down, up, down, up inside EmailCooldownMinutes sent trigger, resolve, trigger, and then nothing. The first "Server Restored" stamped its own cooldown key. The delivered close only cleared the "Server Unreachable" key. So the second recovery was throttled and PagerDuty's second incident stayed open. The AG Replica pair had the same shape. This is unreleased code from #5368.

What changes

• WebhookAlertService: after a delivered PagerDuty send of a firing metric, the closing edge's cooldown key is cleared with ClearMetric. The firing metrics are Server Unreachable and AG Replica Disconnected. The flag must be on. The condition matches the existing clear in the other direction (PagerDuty delivered without error, flag on). ClearMetric is sticky against history re-seeds, so a pre-clear history row cannot throttle the recovery again.
• AlertFamily.TryGetRecoveryOf: the reverse lookup of RecoveryPairs.
• The code lives in the shared Notifications project, so Lite and Darling both get it. Tests are in both products.
• Generic webhook {{dedup_key}}: code unchanged. It stays on per-state keys with the flag on or off, because its send uses the default autoResolve = false. I searched tooltips, Lite and viewer XAML, Darling config docs, README and docs/. None of that text says it follows the flag. Two code comments did say so (the DerivePagerDutyDedupKey remarks and the generic send's key comment). I corrected both. The CHANGELOG buffer entry for PagerDuty: de-duplicate edge-type alerts into one incident #5368 does not make the claim either.

Test plan

• RED at origin/dev head with the new tests and no fix. Lite 4 failed (server and AG, with and without a history store). The fourth send came back Throttled. Darling 2 failed (server and AG). The auto-resolve-off controls passed before and after.
• With the fix: Lite 461 passed, 0 failed. The classes were PagerDutyWebhookTests, IncidentCooldownTests, WebhookCooldownSeedTests, AlertDeliveryChannelTests, AgAlertEvaluatorTests, PerEventNotificationTests, Census, Ratchet, Hygiene, Pin, EffectiveStartUtcTests and related. Darling 1250 passed, 0 failed, 31 skipped. The classes were AlertDeliveryChannelTests, PagerDutyAutoResolveRungTests, Census, Ratchet, Hygiene, Pin, EffectiveStartUtcTests and related.
• Both builds 0 warnings.
• Full suites not run locally. CI decides.

Part 2: the AG cooldown per replica

What was wrong

With auto-resolve on, each AG replica is its own PagerDuty incident, because the dedup key carries the replica. The webhook cooldown key was still per server and metric. Take two replicas of one AG with all four edges inside the window. The first replica's alert stamped the shared key. So the second replica's down alert was throttled and its incident never opened. The first replica's recovery stamp likewise throttled the second replica's resolve, which was a lost resolve.

What changes

• WebhookAlertService.PairedReplicaScope is the one place the rule lives. Auto-resolve is on, the metric is either edge of the AG Replica Disconnected / Reconnected pair, and the alert carries AgReplicaIdentity. The PagerDuty dedup key and the webhook cooldown key both read it. Anything else gets the same keys as before, byte for byte. That covers the flag off, the server pair, and an AG edge with no identity.
• IncidentCooldown.EvaluateAsync and ClearMetric take an optional scope that joins the metric-level key only. The firing and the recovery of the AG pair are both scoped. So one replica's recovery and the clears from part 1 touch only that replica's entries. That also removes the "sibling is re-armed" side effect the old "Accepted" comment described.
• A scoped key is not seeded from the alert history, because the history has no replica to match on. After a restart a replica's key starts as a first notice. An alert can re-post once inside a window. It cannot swallow one.
• The "Accepted" comment and the ClearMetric doc now describe what the code does. The AgReplicaIdentity doc says the cooldown reads it.
• Email is not affected. EmailSendCore holds its own IncidentCooldown with its own key space and passes no scope. So email keeps one notice per server and metric inside the window.
• Teams, Slack and the generic webhook share the webhook cooldown. With the flag on, each replica now gets its own outage notice and its own recovery notice in those three channels. That is intended: one incident per replica.
• Lite and Darling both get this, through the shared PerformanceMonitor.Notifications.

Test plan (part 2)

• RED without the fix, new tests only. Lite 2 failed (SendPath_AutoResolveOn_TwoReplicasDownThenUpInsideTheCooldown_ResolvesBothIncidents, with and without a history store). Darling 1 failed (TheDeliverer_ResolvesBothReplicaIncidents_WhenTwoReplicasFlapInsideTheCooldown_WithTheFlagOn). The collections differed at index 1, the second replica's down. The two auto-resolve-off controls passed before and after.
• With the fix: Lite PagerDutyWebhookTests 41 passed. Lite alert, webhook, cooldown and PagerDuty classes 715 passed, 0 failed. Darling alert, webhook, cooldown and PagerDuty classes 2181 total, 1 failed, 64 skipped. The one failure (StatementFilterAlertTests.Apply_AFourMegabyteReport_FinishesInsideTheBudget) is a timing budget. It passed when run alone (14 of 14).
• Census, Ratchet, Hygiene, Pin and EffectiveStartUtcTests: Lite 354 passed, Darling 1153 passed (31 skipped), 0 failed.
• Added IncidentCooldownTests.Scope_SplitsTheMetricLevelKey_ClearsAndSeedsPerScope for the scope on its own.
• Both builds 0 warnings.
• Full suites not run locally. CI decides.

The changelog entry already covers the fix in one line.

Review fixes

• A restart in the middle of an outage no longer throttles the second incident. The cooldown's history seed now also reads the other edge of the pair (down or up). It skips its own stored send when the other edge sent more recently. It runs only for the server pair and an AG edge without a replica identity. It needs auto-resolve on and PagerDuty routed. It costs one extra seed read. A fresh cooldown over the same history now sends the second recovery (down, up, down, restart, up) and the second firing (down, up, restart, down). A scoped replica key is still not seeded.
• The ClearMetric doc no longer calls the restart case accepted. It says what the seed does now.
• The comment on the triage link no longer says the generic {{dedup_key}} token follows the flag. The link and PagerDuty follow it, the token does not. The token code is unchanged.
• The auto-resolve flag is read once per delivery, from the one delivery snapshot taken before the cooldown. The scope, the seed and both clears use that value. The triage link now reads the same snapshot.
• Round 2: Lite now reads the flag once per delivery too. Lite has no snapshot, only the live settings, so before this the send read the flag a second time, mid-post. That value now goes into the triage link and into the PagerDuty send, for both its dedup key and its event action. If the flag is saved while a post is in flight, the post still uses the value read first. The old Lite caveat comment is gone.
• The per-replica cooldown scope and the pair-aware seed apply only when the flag is on AND PagerDuty has a destination for that alert. With no PagerDuty key, or a key routed only to other metrics, every key is as on dev. To know this before the cooldown, the delivery snapshot and the route (a pure function) are now resolved ahead of it.
• Round 2: the pairedEdge doc now says the seed is looser than the in-memory clear. After a restart the seed counts any delivered webhook row of the other edge, in Teams, Slack or the generic webhook. The in-memory clear needs a delivered PagerDuty post. So a restart after a failed PagerDuty close can let one extra notice through. It never swallows one, and PagerDuty folds it into the open incident. Querying PagerDuty rows alone changes both stores, so I left it.
• Round 2: the pairedEdge doc now also says it is passed only when PagerDuty is routed for the alert, not just when auto-resolve is on.
• Round 2: when the scope applies, Teams, Slack and the generic webhook keep one cooldown per replica. Each replica gets its own outage notice and its own recovery notice in those three channels. Email stays per server. The auto-resolve tooltip says this in Lite and in the Darling viewer (the only two copies), and its "This applies when" sentence now has a clear subject. The changelog entry says it too.
• Two stale comments in IncidentCooldown are fixed. One was the shape of a cleared key. The other was that ClearMetric is now also called for the recovery metric after a delivered firing.
• The changelog entry now says the fix holds across a restart. It says the per-replica cooldown needs PagerDuty set up for the alert. It says that cooldown is shared with the chat and generic channels.
• New tests from the first round: a flag-off "up, down, up" where the recovery stays throttled. A PagerDuty send failure on the second firing where the recovery stays throttled. The two restart cases, a flag-off restart control, a cooldown-level pair-aware seed test, and a Darling test for three PagerDuty setups. Those setups are no key, a key routed to another metric, and a key routed (1, 1 and 2 Teams posts).
• Round 2: PagerDutyWebhookTests.SendPath_AutoResolveFlagChangesAfterTheCooldownRead_PostsWithTheValueReadFirst makes the flag read true once and false after. The second send must still be a resolve under the pair's key.
• Round 2: IncidentCooldownTests.PairedEdge_Seed_TreatsEqualTimesAsNotSuperseded pins the tie rule. Both edges stored at the same instant still throttle.
• Round 2: AlertDeliveryChannelTests.TheDeliverer_AfterARestart_SkipsTheSupersededSeed_OnlyWhenPagerDutyIsRoutedForTheAlert is Darling's restart pin. History holds down at t-3 and up at t-2, then a new service starts. With no PagerDuty key or a key routed to another metric, the second outage stays throttled (0 Teams posts). With PagerDuty routed it posts (1).
• The two Darling flap tests use the existing DelivererFor helper. It takes an optional PagerDuty key, Teams URL and routes, and now an optional history store.

Test plan (review fixes)

• RED, each gate removed on its own, then restored. Pair-aware seed off in the service: 2 Lite restart tests failed. Flag gate removed from the new clear: the flag-off up-down-up test failed. PagerDuty-error gate removed: the failed-post test failed. Route gate removed from the scope (flag only): 2 Darling cases failed (no key, other-metric route). Seed skip disabled in the cooldown: the cooldown unit test failed, plus the 2 restart tests.
• RED for round 2, each change put back to the old shape and then restored. The send reads the live flag again for its dedup key and event action: the Lite flag-change test failed. >= instead of > in the seed comparison: the tie test failed. The route gate on pairedEdge changed to the raw flag: 2 of 3 Darling restart cases failed (no key, other-metric route).
• With the round 2 fixes: Lite PagerDutyWebhookTests and IncidentCooldownTests 68 passed. Lite classes matching Alert, Webhook, Cooldown, Census, Pin and PagerDuty: 1077 passed, 0 failed. Darling AlertDeliveryChannelTests 59 passed. Darling classes matching Alert, Webhook, Cooldown, Census, Ratchet, Hygiene, Pin, EffectiveStartUtcTests and PagerDuty: 3301 total, 2 failed, 95 skipped. The two failures were StatementFilterAlertTests.Engine_AFourMegabyteReport_ReachesTheDelivererFilteredNotWithheldWhole and StatementCollectionCensusTests.ABlockingRingBufferOfAboutFourMegabytes_IsJudgedWellUnderTheBudget. Both are timing budgets under load. They passed when run alone (59 of 59 with their classes).
• Both builds 0 warnings.
• Full suites not run locally. CI decides.