Repository navigation
High CPU, Deadlocks, Poison Wait and Volume Free Space deliver Warning-to-Critical escalations at once (#5715, part A) - #5751
Conversation
|
Two notes on the body itself rather than the code, so whoever reads this PR from the top is not misled. The part-A "Calls to check" bullet is stale. It says:
The backoff section further down is the correction — The read-failure retry is unbounded, and it is the only one of the three store-read seeds that is. Everything else in both accounts matched the tree; the one substantive finding is on |
There was a problem hiding this comment.
Verdict: CHANGES REQUESTED
The finding is inline on PerformanceMonitor.Alerting/AlertEngine.cs:636, with two body-level notes in a separate comment.
Claims
- VERIFIED — four own
TierEscalationinstances, keyed like their cooldown dictionaries —AlertEngine.cs:1496-1499, all keyed onsnapshot.ServerKey(:460) - VERIFIED — the four gate rewrites are the body's table —
AlertEngine.cs:1079,1846,2013,2878-2880 - VERIFIED — deadlocks keep the new-count test, a Critical rate being a higher count —
GradeDeadlockFire(:3951) is monotone in the count; the watermark is the last fire's count (RollingCountAlertGate.cs) - VERIFIED — email, webhook,
IncidentCooldown,RepeatDeliveryBudget, viewer toast needed no change —EmailSendCore.cs:155,WebhookAlertService.cs:261,IncidentDeliveryFilter.cs:181/256,AlertToastCoordinator.cs:169-172 - VERIFIED — Lite has no toast cooldown of its own —
IsOptedIn's only product-side reader isAlertToastCoordinator.cs:170 - VERIFIED —
HasOpenStreakusesRecordFailure's lapse rule —FailedSendBackoff.cs:132-134vs:110-113 - VERIFIED — all six sites pass their
AfterFirefamily string, spelled alike —AlertEngine.cs:4067vs:4093; families matchAfterFireat1174,1917,2068,2251,2425,2913 - VERIFIED — an exact fire/resolution tie goes to the resolution —
AlertHistoryEscalationSeed.cs:49 - VERIFIED — Volume Free Space persists a tier only for a Critical fire —
AlertEngine.cs:2891-2899 - VERIFIED — the seed runs first in
EvaluateCoreAsync, once per (key, family), window = the cooldown,Restoreing only with no entry —AlertEngine.cs:473,619-653 - VERIFIED — a failed read logs, counts as "escalation tier seed", seeds nothing, retried per family —
:647-652plus the census bump 41→42 / 17→18 - VERIFIED — both real stores override the default member, both hosts wire the reader —
DuckDbAlertHistoryStore.cs:348,PgAlertHistoryStore.cs:175,Lite/MainWindow.xaml.cs:342,DarlingWorker.cs:7359 - VERIFIED — Lite writes no resolution rows —
Lite/MainWindow.AlertEngine.cs:445 - VERIFIED — the six titles are constants with a no-bare-literal pin, the tier read as
AlertHistoryRowSeverity.FiredAtreads it —EscalationSeed.cs:23-28,EscalationRestartSeedTests.cs:322-336,AlertHistoryEscalationSeed.cs:53onAlertContext.cs:890 - UNVERIFIED — "both projects build, 0 warnings" — needs a Windows build; no
dotnet buildhere - UNVERIFIED — every test count (365 / 1616 / 668 / 67 / 1092 / 93 / 501 / 75) — needs a Windows run of both suites
- UNVERIFIED — the red/green probes (6 of 6; 7 of 14 then 15 of 15) — needs a mutated tree, built and run
- UNVERIFIED — "the 8 live PostgreSQL tests passed" — needs
DARLING_TEST_PG; the class skips without it (EscalationSeedLiveStoreTests.cs:548) - REFUTED — "Cost: a Critical delivered sooner than the cooldown would have, never one held." — a seeded Critical can only hold.
AlertSeverityLevelis{Warning, Critical}(AlertSeverity.cs:17-21),IsEscalationistier > highest(TierEscalation.cs:72), andRecordkeeps the higher tier (:85), so a Critical seed survives the next incident'sRecord(key, Warning)and cancels that incident's worsening. Walked through onAlertEngine.cs:636 - REFUTED — "A legacy
undeliveredrow … is counted; the cost is a Critical delivered sooner, never one held." — same mechanism; such a row carrying Critical is kept byAlertHistoryEscalationSeed.cs:29and blocks the next escalation - REFUTED — part A's "A failed Critical retries on every evaluation, not on the failure backoff." —
EscalationOpensGate(AlertEngine.cs:4061-4073) refuses the bypass while a streak is open. The body's own backoff section is the correction: a stale sentence, not a tree defect
Riskiest lines
PerformanceMonitor.Alerting/AlertEngine.cs:636— decides what crosses a restart; aCriticalhere is pure downside, andHighest(key) is nullis also true afterClose, so a late-succeeding read can seed into an incident this process already finished — UNPINNED (AfterARestart_ACriticalFromHistory_IsNotAnEscalationpins the same-incident case and stays green either way)PerformanceMonitor.Alerting/AlertEngine.cs:4066— one gate for all six families; misreading the streak either re-fires a Critical every 30 s with every channel down or drops the bypass — pinned byTwoTierEscalationTests.AnEscalationWhoseEveryChannelFailed_BacksOffLikeAnyFailedSend_AndADeliveredSendEndsTheStreakand itsRunnableQueueEscalationTests/AttentionAlertTeststwinsPerformanceMonitor.Alerting/AlertEngine.cs:2878-2880— an escalation now skipsLowDiskAlertGate.ShouldAlert, the only brake on a standing breach re-paging — pinned byTwoTierEscalationTests.VolumeFreeSpace_AnEscalationPassesTheWorseningCheck_WhenTheFreePercentBarelyMoved
Traps near this diff
- "A default interface member compiles a parity gap away … its members are required." (#3640) — does not apply — both real stores override
GetLastFiredTierAsync(DuckDbAlertHistoryStore.cs:348,PgAlertHistoryStore.cs:175) under reflection pins (EscalationSeedHistoryStoreTests.TheStoreOverridesTheInterfaceDefault,EscalationRestartSeedTests.cs:323); the only other implementor is the deprecatedJsonAlertHistoryStore - "A history row wearing the colour its alert's NAME implies rather than the tier it fired at." (#3635, #3680) — applies —
EscalationSeed.cs:56-58resolves a tierless row by NAME viaAlertMetricClassifier.IsCritical, true for "Deadlock"/"Poison" names (AlertMetricClassifier.cs:100-106), so such a row seeds Critical and cancels those families' escalation - "An alert evaluated every ~30 s against an observation that changes once per COLLECTION re-fires on every cooldown expiry." (#3579, #3636) — does not apply — Poison Wait keeps
hasFreshCollection(AlertEngine.cs:2004-2013), deadlocks the new-count test (1846-1848), High CPU a gate edge needing a freshCpuSampleTimeUtc(896-898); Volume Free Space allows one escalation per incident (2887) - "An alert that fires on ONE breaching sample and resolves on the next … sit behind
AlertPersistenceGateat K consecutive collections." (#3692) — does not apply — the gate is untouched andFireCpuAsyncis reached only from a gate edge (AlertEngine.cs:928-933) - "Did I send is not was it delivered … Gate on a delivery stamp in the store." (#3633) — does not apply — part B is that fix: the tier comes from
config_alert_log(AlertEngine.cs:619-653) and a fire whose every channel failed is dropped (AlertHistoryEscalationSeed.cs:29) - "Lite and Darling ship in parity … a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding." (#3594) — does not apply — shared engine, both stores on the shared predicate/resolver, both hosts wired (
Lite/MainWindow.xaml.cs:342,DarlingWorker.cs:7359), both halves tested - "A test that reads the clock twice measures the CI scheduler … One anchor owns both ends of an interval." (#3676) — does not apply — every new test drives one injected anchor (
TwoTierEscalationTests.cs:801,EscalationRestartSeedTests.cs:151,EscalationSeedLiveStoreTests.cs:516)
Divergence (blind read vs PR body)
Three places, all one direction — the body understates what a seeded Critical can do.
- The body twice bounds the seed's cost at "a Critical delivered sooner … never one held", and lists the Lite no-resolution-row case under Double-check. That case is a real hole: a Critical seed cannot create an escalation and can only cancel the next one, and on Lite the read's resolution arm is structurally dead, so a closed incident's Critical stays readable for a full cooldown.
- The
AlertMetricClassifier.IsCriticalfallback is presented only as what makes Volume Free Space's tierless Warning rows seed; it equally makes a tierless Deadlocks/Poison row seed Critical, the opposite effect, unmentioned. - Part A's "A failed Critical retries on every evaluation" describes a tree this PR no longer ships; the backoff section is its own correction.
No behaviour attached: "a test pins that both real stores override it" is two tests in two projects, the Darling one named BothRealStores… over a one-element array.
Reviewer checklist
- Build both test projects on Windows and run the cited counts — the new theories lean on
AlertEngineTests.Harnessmembers (h.Muted,h.Settings.RunnableQueueEnabled, the mutableVolumeFreeSpaceInfo.FreeMbatTwoTierEscalationTests.cs:1101) only a compiler can confirm - Re-run the part-B red/green probe after narrowing the seed to Warning-only, and confirm
AfterARestart_ACriticalFromHistory_IsNotAnEscalationstill passes - Run
EscalationSeedLiveStoreTestswithDARLING_TEST_PGset — the only check of the naive-UTC window bound (PgAlertHistoryStore.cs:196) and the microsecond ties on a real server - Add the uncovered case: no resolution rows, a Critical fire inside the window, then a fresh Warning-to-Critical incident, asserting the Critical still carries
EscalationBypassesCooldown - Watch the "escalation tier seed" counter against a store unreachable at startup — the retry is unbounded (
AlertEngine.cs:641records only on success)
… started, per-family tierless rule, bounded retry (#5715 round 1) A seeded Critical can never create an escalation and only cancels the next incident's. Seed Warning only; a tierless row seeds a Warning only for Volume Free Space; read only rows before the engine start (exclusive, microsecond) in both stores; stop seeding a key when a read succeeds, the process touches it, or one cooldown passes.
Claude posting for Erik Darling. Round 1 finding fixed in 6b0a60e: the restart seed now applies only a Warning (a Critical seed could only hold the next escalation), only Volume Free Space reads a tierless row as Warning, only rows from before the engine started count, and seeding stops once a read succeeds, the key is touched, or a cooldown has passed since start. The body's cost statements, the stale part A sentence and the test name are corrected. The follow-up review on 6b0a60e hit the 30-turn cap; CI 38378999400 is green.
Claude posting for Erik Darling.
Fixes #5715: part A (the four alerts), the backoff fix (all six alerts) and part B (the restart seed).
Why
High CPU, Deadlocks Detected, Poison Wait and Volume Free Space have a Warning tier and a Critical tier. A Warning that worsened to Critical inside the alert cooldown was held until the cooldown ended. #5709 built
TierEscalationfor Runnable Queue; this applies it to the four older alerts in the sharedPerformanceMonitor.Alerting/AlertEngine.cs, which Lite and Darling both run.What changes
Each alert gets its own
TierEscalationinstance, keyed like its cooldown dictionary. All four follow the Runnable Queue pattern:IsEscalationbefore the gate, the cooldown bypassed on an escalation,EscalationBypassesCooldownset on the context,HighestthenRecordbefore the send (muted fires too),Restorewhen every channel failed, andClosewhen the incident ends.suppressed, Poison Wait's fresh-collection guard and mutes are unchanged. No store reads were added.!suppressed && Cooldown!suppressed && (escalation || Cooldown)GradeCpuFire(value)RollingCountAlertGate(count, threshold, watermark, Cooldown, suppressed)escalation || Cooldownas its cooldown input (its new-count test still applies; a Critical rate is a higher count than the Warning's, so it passes)GradeDeadlockFire(count, rate tiers)!suppressed && freshCollection && Cooldown!suppressed && freshCollection && (escalation || Cooldown)findings[0].Severity(worst finding)!suppressed && ShouldAlert && Cooldown!suppressed && (escalation || (ShouldAlert && Cooldown)); an escalation passes the worsening check tooIsCriticallyLow(worst volume)Other changes:
TierEscalation.s_optedInnow lists all six metric names; its doc comment, theAlertContext.EscalationBypassesCooldowncomment and the engine field comment name them. The restart-gap paragraph is left for part B.IncidentCooldown,RepeatDeliveryBudgetand Darling viewer toast cooldown already read the flag orIsOptedIngenerically, so they needed no code change.Calls to check
_lastLowDiskAlert), its worsening level (_lastAlertedLowDiskPercent) and its tier (the worst volume) are all per server, and it fires one alert per server. The escalation uses the same key as its cooldown dictionary.SystemTrayService/LiteAlertDeliverershow whatever the engine fires), so nothing to change there. Darling'sAlertToastCoordinatorkeys on server and metric and readsIsOptedIn, so the four metrics are picked up through the list.Test plan
New
Darling/Darling.Tests/TwoTierEscalationTests.cs(the existingRunnableQueue*escalation tests live in Darling.Tests), each sequence run for all four alerts through the real engine:s_optedInpinExisting pins updated deliberately in
RunnableQueueEscalationTests: the opted-in list moved to the new pin, theIsEscalation(/EscalationBypassesCooldown =source counts went from 2 to 6, and the "viewer keeps its cooldown for other alerts" test now uses Blocking Detected instead of Poison Wait.AlertEngineTests.DeadlockRowbecameinternalfor the new tests.-trait Stage=Guard: 1616 passed;*Census*,*Ratchet*, DocCommentHygiene, AlertReadFailureSurface: 668 passed, 1 skipped-trait Stage=Guardplus Census, Ratchet, AlertIncidentRender, WebhookCooldownSeed, AlertEngine classes: 67 passedci-select.py map-selectnot run: it needs the CI class-to-file map artifactProposed CHANGELOG entry (whole issue, once part B lands)
SECTION: Fixed
ENTRY: - A Warning that worsens to Critical is delivered at once for High CPU, Deadlocks Detected, Poison Wait and Volume Free Space ([#5715]) - these alerts held the Critical until the cooldown ended. They now deliver it immediately, in Lite and Darling, as Runnable Queue and Client Timeouts already did. All six alerts keep the escalation across a service restart by reading the last tier sent from alert history. An escalation whose every channel failed now waits out the usual 1, 2, 4, 8 minute retry backoff instead of retrying at every check.
Backoff fix: a failed escalation backs off like any other failed send (all six alerts)
Defect. #4752's failed-send backoff lives in
AfterFire: when every channel failed it records a failure in_failedSendsand back-dates the family's cooldown stamp, soCooldownElapsedreopens after 1, 2, 4, 8 ... minutes (capped at the cooldown). Each escalation site callsTierEscalation.Restorewhen every channel failed, so the next evaluation is an escalation again, and the gateescalation || CooldownElapsed(...)skipped the back-dated stamp. With every channel down, a Critical escalation retried at every evaluation with no backoff.Rule. An escalation skips the cooldown only when its family and key have no open failure streak in
_failedSends. While a streak is open it waits forCooldownElapsed(the back-dated stamp is the retry delay). The fire that goes still carriesEscalationBypassesCooldown = true, so the email and webhook cooldowns let it through. Volume Free Space: while a streak is open the escalation waits for the cooldown but still skips the worsening check (ShouldAlert).Code. One helper,
AlertEngine.EscalationOpensGate(family, key, escalation, stamps, now, cooldown), backed by a new read-onlyFailedSendBackoff.HasOpenStreak(family, key, nowUtc)(open = last failure no more than twice the cap ago, the same lapse ruleRecordFailureuses). All six sites call it with the family string they pass toAfterFire: Runnable Queue (RunnableQueueEvaluator.MetricName), Client Timeouts (AttentionEvaluator.MetricName), "High CPU", "Deadlocks Detected", "Poison Wait", "Volume Free Space". The escalation field comment now says Volume Free Space is one alert per server (worst volume) with a per-server key. The restart seed and theTierEscalationrestart-gap paragraph are untouched (part B).Tests. One new test per alert, each running the same six steps:
TwoTierEscalationTests.AnEscalationWhoseEveryChannelFailed_BacksOffLikeAnyFailedSend_AndADeliveredSendEndsTheStreak(theory: High CPU, Deadlocks Detected, Poison Wait, Volume Free Space)RunnableQueueEscalationTests.AnEscalationWhoseEveryChannelFailed_BacksOffLikeAnyFailedSend_...AttentionAlertTests.Engine_AnEscalationWhoseEveryChannelFailed_BacksOffLikeAnyFailedSend_...Red/green. With the helper reduced to the old
escalation || CooldownElapsed, all 6 new tests fail (6 of 6 failed); with the fix they pass (6 of 6).Ran (Darling.Tests / Lite.Tests, both builds 0 warnings). Darling:
*Escalation*,*AlertEngine*,*FailedSendBackoff*,*ChannelFailure*,*Census*,*Ratchet*,*AttentionAlert*,*RunnableQueue*classes: 1092 total, 0 failed, 17 skipped;-trait Stage=Guard: 1616, 0 failed. Lite: the same escalation, engine, backoff, channel-failure, Census and Ratchet classes: 93, 0 failed;-trait Stage=Guard: 501, 0 failed. No whole suites (CI decides). Lite sharesAlertEnginefromPerformanceMonitor.Alerting, so no Lite code differs.(The failed-escalation backoff sentence is folded into the ENTRY above.)
Part B: restart seed
After a restart the engine forgot that a Warning was delivered, so the first Critical was a plain first fire and the email and webhook cooldowns (seeded from history) held it. Now the tier is seeded from the alert history.
Design
IAlertHistoryStore.GetLastFiredTierAsync(serverId, metricName, resolutionMetricName, sinceUtc, countMuted = true, tierWhenRowCarriesNone = null)returns the tier of the newest FIRE row inside the window, or null. Implemented inDuckDbAlertHistoryStore(Lite) andPgAlertHistoryStore(Darling). The interface has a default body that answers null (many test doubles and the deprecated Dashboard store implement the interface); a test in each project pins that its store overrides it (EscalationSeedHistoryStoreTests.TheStoreOverridesTheInterfaceDefaultin Lite.Tests,EscalationRestartSeedTests.ThePgStoreOverridesTheReader_NotTheInterfaceDefaultin Darling.Tests, which checks only the PostgreSQL store).AlertHistoryEscalationSeed.Resolve); the newest fire carries no tier and no fallback was given.countMuted: false), and throttled, unconfigured and folded fires. Not counted: a fire whose every channel failed (alert_sentfalse andsend_errornot null, the engine's ownEveryChannelFailed). A legacyundeliveredrow cannot be told apart from a throttled one and is counted. Only a Warning seeds (round 1 below), so the cost is that a Warning from such a row lets the next Critical inside the cooldown go out at once, as an escalation would.AlertHistoryRowSeverity.FiredAtdoes. Found while testing: Volume Free Space writes a tier only for a Critical fire, so its Warning rows carry none. Round 1: the engine's family table decides the tierless fallback, not the metric name. Only Volume Free Space maps a tierless row to Warning; for every other family a tierless row seeds nothing (the old name arm,AlertMetricClassifier.IsCritical, made a tierless Deadlocks or Poison Wait row Critical, which cancelled those families' escalation).AlertHistoryServerIdentity.SeedScope, asGetLastEmailSentUtcAsyncdoes. Storage precision: tests use whole-microsecond instants; the window bound goes to PostgreSQL as naive UTC.Func(escalationSeedReader, built byEscalationSeed.ReaderFor(store)), wired inLite/MainWindow.xaml.csandDarlingWorker.BuildAlertEngine.EnsureEscalationSeededAsyncruns at the start ofEvaluateCoreAsync, before any family, once per (server key, family) per process; itRestores the tier only when the instance has no entry. The window is the alert cooldown. The six gate lines andFailedSendBackoffare untouched. A failed read logs, counts under "escalation tier seed" on the read-failure surface, seeds nothing, never blocks a fire, and is retried on the next sweep for that family only. The six resolution titles are now constants (EscalationSeed) used by the engine'sAlertResolutionsites, with a pin that no bare literal returns.TierEscalationrestart-gap paragraph is rewritten to describe the seed.AlertReadFailureSurfaceTestscounted sites 41 to 42, andAlertEngine.cscounted 17 to 18 (the new swallowed-read catch).Red/green. With the
EnsureEscalationSeededAsynccall removed,EscalationRestartSeedTestsfails 7 of 14 (the 3 restart cases for High CPU, Volume Free Space and Runnable Queue, plus the throws, once-per-key, retry, six-metric and counted-failure tests). With it restored, 15 of 15 pass (one wiring pin was added after the probe). For High CPU the restart without the seed is even held outright by the in-engine cooldown that the persisted state re-arms.EscalationSeedHistoryStoreTests(14)GetLastEmailSentUtcAsync, store overrides the default; round 1: exclusive microsecond upper bound, a resolution after the bound still closes an earlier fireEscalationSeedLiveStoreTests(11)EscalationRestartSeedTests(22)EscalationBypassesCooldownfor High CPU, Volume Free Space and Runnable Queue; no-reader control; a Critical from history is not an escalation; the seed covers all six metric names with titles and window; title-literal pin; a throwing reader leaves the fire unchanged and is counted; reader called once per key and family over many sweeps; a failed read is retried for that family only; the seed stops once the process recorded the tier; both hosts wired; round 1: see the list belowRun (this branch, rebased onto the part A backoff commit). Darling.Tests:
*Escalation*75,*AlertEngine*294,*AlertReadFailure*27,*Census*548,*Ratchet*15,*FailedSend*123,-trait Stage=Guard1616, all passing; the 8 live PostgreSQL store tests passed on a throwaway cluster (since stopped). Lite.Tests:*Escalation*11,*AlertReadFailure*2,*Census*67,*StoreRoundTrip*12,-trait Stage=Guard501, all passing. Both test projects build with 0 warnings. After the final rebase I reran the non-live classes only (the 9 live tests skipped, PostgreSQL already stopped). Not run: whole suites.Double-check: the Volume Free Space tierless-row fallback (now the only one); legacy
undeliveredWarning rows counted; the Lite no-resolution-row case (a Warning seed from a closed incident, see the cost above); Client Timeouts passingcountMuted: false; the engine start instant is taken at construction.Round 1 fixes (review of 2026-10-13)
The bot found that a seeded Critical can never create an escalation (
IsEscalationistier > highest) and, becauseRecordkeeps the higher tier, can only cancel the NEXT incident's Warning-to-Critical escalation. Fixes:EnsureEscalationSeededAsyncdrops anything else; the reader passes the row's own tier through, and a comment onEscalationSeed.ReaderForsays so). A repeated Critical after a restart is a plain first fire, which the history-seeded email and webhook cooldowns already hold.EscalationSeedQuery.TierlessRowIsWarningcomes from the engine's family table: true only for Volume Free Space. Every other family's tierless row seeds nothing. The name classifier no longer decides it._engineStartUtc). The query carriesUntilUtc; both stores takeuntilUtcas an exclusive bound (alert_time < $4). Both bounds go throughAlertHistoryEscalationSeed.TruncateToMicroseconds, so a bound and a stored row compare at the column's precision; the tie rule is unchanged, and the resolution lookup is unbounded (a resolution after the bound still closes an earlier fire).TierEscalation.IsTouched, becauseHighest(key) is nullis also true afterClose); ornow - engineStart >= cooldown(every pre-start row is then outside the window). With the store unreachable, reads stop after one cooldown instead of failing on every sweep forever.ThePgStoreOverridesTheReader_NotTheInterfaceDefault(it ran over a one-element array under aBothRealStores...name).Red/green (mutating the engine or store, then restoring; all in
EscalationRestartSeedTestsunless noted):TheUpperBound_IsExclusive...,TheUpperBound_IsMicrosecondPrecise...). The PostgreSQL pair was not mutated; it passes live.Run. Both test projects build with 0 warnings. Darling.Tests:
*Escalation*85 (includes the 11 live PostgreSQL store tests, run on a throwaway cluster, now stopped),*AlertEngine*294,*AlertReadFailure*27,*TwoTier*31,*RunnableQueue*139,*AttentionAlert*38,*Census*548,*Ratchet*15,*AlertHistory*120,*PgAlert*54,*DocCommentHygiene*78,*SlowClassCensus*7,-trait Stage=Guard1616, all passing. Lite.Tests:*Escalation*14,*AlertReadFailure*2,*RunnableQueue*38,*Census*67,*AlertHistory*95,-trait Stage=Guard501, all passing.AfterARestart_ACriticalFromHistory_IsNotAnEscalationand the Warning-seed theories stay green. Not run: whole suites.