Never answer one cooldown key's question with another key's history (#3456)

[erikdarlingdata]

Fixes the delivery-correctness half of #3456 — the throttle-instead-of-deliver mechanism and the first-notice suppression. The roster half of that issue's acceptance (a self-alert burst folding into carrier + roster) needs cooldown identity that survives the integer server_id column, which is a schema-shaped follow-up argued below and now filed as #3458; it is deliberately not attempted here.

Both history stores mapped the string server key onto config_alert_log's integer server_id with int.TryParse(...) ? sid : 0, on both sides of the table. On the write side that is a tolerable collapse: the row must be recorded somewhere, the column is an integer, and 0 is where every non-integer key lands. On the read side it was the defect. The restart seed asked "when did THIS key last deliver?" and got back MAX(alert_time) over EVERY collapsed key's rows — the fleet-wide last successful send of the metric — because the whole self-alert family's keys are non-integers by construction (cost:<id>:<collector>, the Availability Group composites, the fixed sentinels) and every one of their rows sits in the same bucket. The per-fingerprint cooldown #1154 built therefore degraded, for exactly this family, back into the fleet-wide per-metric mutex #1154 removed — one layer down, where the alert log's own rows could not show it until #3446's dispositions made the shape legible.

Measured live, twice, identically (the issue's banked reproductions): two servers' Collector Cost Regression fires for the same collector land 3.7 ms apart. The first seeds against the fleet-wide last send — 60m07s prior, outside the window — and delivers. The second seeds 3.7 ms later against the first's just-written row and renders throttled, despite its own last delivery being 2h00m13s prior. The 60m07s figure equals the fleet-wide MAX(alert_time) for the metric to the second, which is what identified the seed as the mechanism. Retroactively this is also the pre-#3446 exactly-one-winner bursts: every burst member after the first seeded against the winner's row.

In severity order, per the issue: a first notice could be suppressed by another server's send — an unannounced incident, the class #1154 exists to prevent and the line RepeatDeliveryBudget's own remarks say must not be crossed; #3436's roster never engaged for this family, because the throttle returns before the budget evaluates; and the throttled disposition inverted its meaning, reporting re-tell suppression of an incident it was actually suppressing on other incidents' sends.

The change

The mapping now lives in one shared type, AlertHistoryServerIdentity, used by both stores — the same arrangement AlertValueParser already gives the value columns, and for the same reason: two stores answering an identity question with separately-maintained parses is the drift risk, and this question is the one the issue turned on.

• StorageId is the write side, unchanged in behavior: an integer key stores its own id, everything else collapses to 0. Centralized so the rows the bucket absorbs are demonstrably the same ones the seed refuses to read, and so the follow-up changes one line per direction.
• SeedScope is the read side and the fix: it declines (null — no seed) any key whose rows are not distinguishable by server_id alone. That is the unparseable keys, and also the literal key "0": the bucket's rows are mixed by construction, so an answer from it is cross-key history even for a caller whose key genuinely is 0. The bucket is write-only. A declined seed reads as a first notice and posts — the direction the delivery doctrine already names ("fails toward posting"): the cost of failing this way is a post; the cost of the old way was an unannounced incident.

The invariant, pinned: the seed never answers one key's question with another key's history. Even the asking key's OWN rows are unrecoverable from the bucket — nothing in a collapsed row says which key wrote it — so the honest answer for the whole family is "no answer", which is the pre-seed (#1145) behavior: the in-memory stamp governs within the process lifetime.

PgAlertHistoryStore.ReadMaxAlertTimeAsync serves all three seed reads, so one early return fixes the webhook seed (the measured defect), the email seed (same mechanism, same fix), and GetLastAlertTimeAsync. Lite's DuckDbAlertHistoryStore carried the same parse in all three of its reads — the twin the Pg store's class doc claims query-for-query parity with — and takes the same change, which keeps that claim true.

Other callers of these reads, checked: GetLastAlertTimeAsync feeds the analysis notification cooldown and the #2716 Tier-0 predictor seeds. Every one of those callers passes an integer key exclusively — Lite resolves f.ServerId.ToString(), Darling resolves finding.ServerId and snapshot.ServerKey, which is the storage-name hash rendered as a string — so their behavior is unchanged. The only populations whose reads change are unparseable keys (previously: garbage answer from the bucket; now: no answer) and the literal "0" (previously: the bucket; now: no answer), both of which resolve toward posting.

Parse-to-0 fallbacks in the delivery/history path, named and left alone: both stores' RecordAlertAsync (the write-side collapse, now spelled StorageId and kept deliberately — a row must be recorded, and the seed no longer reads the bucket); Lite's EmailAlertService.SendFindingAlertAsync (collapses alert.ServerId to an int parameter; its only producers pass integer finding ids, so the fallback is dead in practice and it feeds a send path, not a seed); and the deprecated Dashboard's JsonAlertHistoryStore, which compares the string key directly and never had this defect. DarlingWorker's resolveServerOverride already falls back to null rather than 0 — the pattern this change extends to the seeds.

What this deliberately does not do

The issue's acceptance has three clauses, and this ships two: a first-notice self-alert is never throttled by another server's send, and the banked pairs' shape re-runs green (both pinned below). The third — a multi-server self-alert burst delivering one carrier with the siblings folded onto its roster — is not met, and cannot be met by any seed that declines: with no per-key history, an evicted or post-restart key reads as a first notice, and a first notice is exempt from folding by #1154's rule, correctly. So the hourly multi-server pairs that identified this defect now deliver two full cards rather than one throttled and one delivered. Nothing is lost — the old build lost announcements, this one spends posts — but #3436's fold stays structurally unreachable for the self-alert family, and the family keeps no cooldown continuity across a restart.

Restoring both needs seed-and-stamp identity that survives the integer column. The two candidate shapes are weighed in #3458: hashing the string key into the column on both sides is two lines but mints ids outside the registered population — a hash of an arbitrary composite can land ON a registered server's id and interleave the family's rows into that server's per-server reads and dismiss keys, a new and silent defect class — while a rung persisting the string key alongside the column carries identity instead of inferring it and changes no reader. The rung shape is the recommendation. No rung is claimed here, because this change needs no schema and a rung ships with the change that needs it.

Tests

AlertHistoryServerIdentityTests pins the pure mapping: integer keys answer the same id on both sides (including negative hashes), the self-alert shapes write to the bucket and read nothing, the literal "0" writes to the bucket and still reads nothing, and the invariant is stated as a property — the seed either declines or answers with the write side's own non-zero id — so a future edit that lets the halves drift fails by name.

StoreRoundTripTests.SeedReads_ForUnparseableServerKeys_DeclineRatherThanReadTheCollapsedBucket runs the real DuckDB store, like its siblings, because the refusal is a line of SQL scoping plus a guard and a text assertion would pass against any code containing the right words. It pins the write-side collapse first — the refusal only means something if the rows really land at 0 — then the seam the defect lived in: a same-metric sibling key's seed reads null against a fresh bucket row, as does the writing key's own, as does "0"'s, for webhook, email, and unfiltered reads. The control records under "7" and asserts "7" still seeds while "8" does not, so the #1145/#981 restart guarantee is measured, not assumed.

WebhookCooldownSeedTests gains the service-level half, following that file's dead-URL discipline (a suppressed post never touches the network; an attempted one fails observably): a first notice on an unparseable key comes back Failed — attempted — with the seed consulted, never Throttled by another key's fresh send; the integer-key control is one fixture where the only difference between Throttled and Failed is whose row the store holds. The store double models the integer column through the shared mapping rather than keying on the caller's string, which is the distinction the whole issue turns on: a string-keyed fake keeps every key's history separate and the defect cannot reproduce against it.

RepeatDeliveryBudgetTests.OnTheWire_TwoSelfAlertSiblingsMillisecondsApart_TheSecondIsNotThrottledOffTheFirstsSend is the banked shape end to end: two same-metric self-alerts on different unparseable keys through the real deliverer, real cooldown, and a column-faithful store double whose RecordAlertAsync collapses exactly as the real stores do — so the first delivery writes the row the second's seed would have read. Both post; both history rows say webhook-delivered; both rows demonstrably landed in the bucket. On the pre-fix seed the second call reads the first's just-written row and comes back throttled, verified out of band against the built assembly, so the fixture discriminates the mechanism rather than passing by shape. The deferred half is asserted rather than left silent: neither card carries a roster, because both siblings are first notices — the pin says nothing is dropped, and deliberately does not say the burst folds.

Tests cannot run on this lane's platform (net10.0-windows test projects); every touched assembly builds clean, and the mapping, the composed cooldown behavior (both siblings post; integer controls throttle and pass correctly), and the pre-fix reproduction were executed out of band against the built PerformanceMonitor.Notifications assembly.

A separate preparatory commit makes the one-line change CollectorCostDigestTests' factory reserved for the #3441 merge — the #3441 and #3448 lanes crossed and Darling.Tests has not compiled on dev since the second of them landed. BaselineP95MsPerRun is pinned to each fixture's mean so ThresholdMsPerRun resolves to the pre-#3441 bound exactly and every digest fixture keeps asking the routing question it was written to ask.

The branch also carries the one-line cure for #3457 — dev's Darling.Tests compile break from the #3441×#3448 cross-merge (the digest factory passing nine positional arguments to a CostRegression that #3441 gave ten): the named argument the factory's own comment reserved, ordered first on the branch so every commit after it builds against a green dev. ci-sentinel filed that issue from the red run; this is its fix.

[claude]

Review summary

Reviewed the diff against CONTRIBUTING.md (C# style, Two-Store Parity section) with a focus on correctness, Lite/Darling parity, security, and performance. No SQL was touched, so the T-SQL style rules don't apply here.

Verdict: looks correct and solid. The fix is narrowly scoped, well-tested, and centralizes the identity mapping exactly where the "Two-Store Parity" guidance in CONTRIBUTING.md says duplicated parses are a drift risk.

What I verified:

• Mechanism: confirmed DarlingSelfAlertEvaluator builds keys as $"cost:{regression.ServerId}:{regression.CollectorName}" — a genuinely unparseable string — which validates the described defect (fleet-wide MAX(alert_time) bleed via the collapsed server_id = 0 bucket).
• Parity: AlertHistoryServerIdentity.StorageId/SeedScope is applied identically in PgAlertHistoryStore (write + ReadMaxAlertTimeAsync, which backs all three seed reads) and DuckDbAlertHistoryStore (write + all three Get*SentUtcAsync/GetLastAlertTimeAsync methods). Both sides got equivalent new tests (RepeatDeliveryBudgetTests for Darling; StoreRoundTripTests + WebhookCooldownSeedTests for Lite). No parity drift found.
• SeedScope correctness: excludes both unparseable keys and the literal "0" (since that bucket is inherently mixed), returns null → callers treat as "no seed" → first notice → fails toward posting. Matches the stated invariant and is pinned by a property-based test (TheSeedEitherDeclines_OrAnswersWithTheWriteSidesOwnNonZeroId).
• JsonAlertHistoryStore (deprecated Dashboard) — confirmed it compares the string key directly (entry.ServerId != serverId) and never had this defect, consistent with the PR description.
• No behavior change for the write side (StorageId mirrors the prior inline int.TryParse(...) ? sid : 0), and NumberStyles.Integer + CultureInfo.InvariantCulture is equivalent to the previous implicit-culture int.TryParse for this use (no thousands separators are accepted either way) — so this is a safe refactor, not a behavior change, on the write path.
• Performance: the decline path returns before opening a DB connection, which is actually a small win over the old behavior (no more wasted round trip / scan against the server_id = 0 bucket for the self-alert family).
• Security: no new SQL surface — same parameterized queries ($1/$2, AddWithValue), no string concatenation of user input.
• Test double fidelity: both new store doubles (CollapsingColumnHistoryStore in Darling, CollapsedColumnHistoryStore in Lite) correctly model the real stores' column collapse via the shared mapping rather than keying on the caller's string — which is the detail that makes them actually able to reproduce the original bug pre-fix.

One minor/non-blocking observation (out of scope for this PR's diff, so noting here rather than inline): Lite/Services/EmailAlertService.cs:142 (SendFindingAlertAsync) still has its own inline int.TryParse(alert.ServerId, out var sid) ? sid : 0 instead of the new AlertHistoryServerIdentity.StorageId. The PR description explains why it's left alone (write/send path only, producers currently always pass integer ids), which is a reasonable call — just flagging it as the one remaining duplicate of this parse in case a future producer starts feeding that method an unparseable key.

🤖 Generated with Claude Code