Forced Plan Failing fires once per observation, not once per cooldown: the adapter returns the collection that produced the rise and the engine remembers which one it already reported (#3579)

[erikdarlingdata]

The mechanism

CheckForcePlanFailuresAsync documented itself as "a standing condition … while a plan keeps failing it re-fires on the cooldown, mirroring the database-state family." That is the right contract when the input is re-measured on every pass. This condition's input is not. ForcePlanFailuresSql ranks one plan's sightings by collection_time and returns the plan when the NEWEST sighting's force_failure_count is above the PREVIOUS one's — a fact about two collections. It becomes true the instant the newer collection lands and then stays byte-identical until the next one does. The engine re-asks every ~30 s; the cooldown is 5 minutes; the query_store collector's cadence is 5 minutes by default and ~15 on the store that found this. A cooldown shorter than the cadence expires several times against the same two rows, and at pass granularity "still failing" and "no new data yet" are indistinguishable. Nothing carried the observation's identity from the adapter to the engine, so the engine took the second for the first, every time.

The evidence

One production store, one plan (#3579). The raw per-collection series was 0, 0, 1 (forced AUTO), 0, 0 at 03:47 / 04:02 / 04:18 / 04:50 / 05:01. Between the 04:18 and 04:50 collections the engine fired six times — 04:18:53, 04:24:31, 04:30:04, 04:35:33, 04:40:49, 04:46:03 — one per cooldown, every card honestly reading New 1 / Total 1, because every pass re-read the same 04:02→04:18 rise. The channel saw two of the six only because webhook-side throttling ate the rest. The arithmetic was right throughout; the repetition was the defect.

The codebase already names this class: "#2704/#2708 — a cooldown timer with no memory of which data point it last fired on." Poison wait took the unrefreshed-source-row guard for it under #2704 (_lastPoisonWaitCollectionTime, on both the SQL Server engine and the PostgreSQL twin). This is that guard at plan grain.

Why this shape

The adapter returns the observation's identity. ForcePlanFailureInfo.ObservedAtUtc is the newer sighting's collection_time, appended as the eighth column observed_at in both SKUs' SQL. Appended, not inserted, so the seven ordinals both readers already bind do not move. It is n.collection_time, already in per_collection's GROUP BY and already the second key column of the #3573 covering index — no new qs. reference, so the Index Only Scan is untouched (the access-path pins re-derive the column list from the statement text and still agree with the index).

The engine fires once per NEW observation that shows a rise. Per plan it remembers the stamp it last fired on and declines to fire a stamp that is not newer, regardless of cooldown. A newer stamp with a rise fires — a plan failing across successive collections still re-fires, each collection being a new observation — and the cooldown still rate-limits those. Recovery is unchanged: the plan stops appearing, the resolution is announced, and the memory goes with it.

One per-plan state, not two. The memory lives beside the identity the condition already kept for the recovery message: _activeForcePlanAlerts now holds a small ForcePlanActivePlan { LastSeen, LastAlertedObservedAtUtc } per plan instead of the bare row. The two fields are deliberately separate memories — LastSeen is refreshed on every pass the plan appears; LastAlertedObservedAtUtc only when it fires. Folding them into one "last seen" stamp was the tempting shortcut and would have been wrong in the direction of silence: a collection landing INSIDE the cooldown would have been recorded as seen and then never fired. There is a pin for exactly that case.

The comparison is "not newer", not "not equal." The contract reads "once per new observation," so an equal stamp is not new and neither is an older one (nothing produces one today — a deleted newest row or a clock step would). A muted fire stamps the observation as it stamps the cooldown: the operator muted the plan, not the engine's memory of it.

Stampless rows degrade toward repetition, not silence. ObservedAtUtc is nullable. A null never matches a remembered stamp, so an adapter that supplies none keeps the pre-#3579 cooldown-repeat — the same stated fallback IAlertStateStore gives a host that cannot persist. Both shipped adapters always supply it; collection_time is NOT NULL in both stores. Pinned so the fallback is a decision rather than an accident of null comparison.

Restart behaviour, stated. The family's state is in-memory (alert_persistence_state is not used by this condition and nothing about it is persisted today), so a restart empties the memory and the first pass after one may re-fire once for an observation still in the two-hour window. That is exactly what the cooldown clock, also emptied, did before this PR — the guard never makes a restart noisier than it was. Persisting it would be a new IAlertStateStore seam for a once-per-restart card; not taken here.

What it does NOT do

• It does not change what counts as a rise, the two-hour window, the > filter, the severity, the card's fields, the mute key, or the recovery text.
• It does not touch the poison-wait guard, the PostgreSQL twins in DarlingWorker, or any other condition. The census below found one sibling with the same shape whose fix is NOT the identical guard; it is named for a follow-up rather than widened into here.
• It does not persist the observation memory (see restart behaviour above).

Census — conditions in AlertEngine.cs that read a stored between-collections fact and re-fire on cooldown

The issue asked for this before writing. "Same shape" = the condition's input changes only when the collector lands a row, AND its cooldown (5 min default) is shorter than the feeding collector's cadence, AND nothing carries the observation's identity.

Condition	Input	Feeding cadence	Cooldown < cadence?	Observation identity carried?	Verdict
Forced Plan Failing	delta between two newest collections	query_store 5 min default, ~15 measured	yes	no → now yes	this PR
Poison Wait	rolling 10-min sum of collector deltas	wait_stats 1 min (delivered cadence lags at fleet load)	can, at load	yes (#2704, NewestCollectionTime)	already guarded
Database File Growth (rise arm)	current − oldest sample in 60-min lookback	database_size_stats 60 min	yes, 12:1	no — DatabaseFileGrowthInfo has no collection_time; the adapter CTE has c.collection_time but does not project it	same shape, not the identical fix — needs a new column in both SKUs' SQL + the info type + the engine, the size of this PR. Named for a follow-up. Its level arm (file ≥ X% of volume) is a standing level and re-fires by design.
Blocking Detected / Deadlocks	rolling-window event count	1 min / 5 min	no	n/a — RollingCountAlertGate watermark on the count (#1091) is an edge, not a cooldown repeat	not the shape
Blocking Wait Time	LEVEL, newest snapshot	1 min	no	SnapshotIsFresh gate	not the shape (level)
High CPU	live ring-buffer sample	per pass	no	#3282 persistence gate counts distinct samples	not the shape
Long-Running Query	live state, newest snapshot	query_snapshots 1 min	no	—	not the shape (level)
tempdb Space	LEVEL, newest row	tempdb_stats 1 min	no	—	not the shape (level)
Volume Free Space / PVS	LEVEL with worsening watermark	1 min / 60 min	PVS: yes	LowDiskAlertGate/PvsAlertGate re-fire only on WORSENING, so an unrefreshed row cannot re-fire	not the shape (edge on worsening)
Anomalous Agent Jobs	newest snapshot vs history	running_jobs	—	SnapshotIsFresh (#1812) refuses stale snapshots	not the shape
Failed Jobs	newest failed run	live msdb	—	run-time watermark (#1145)	not the shape
Database State	state deviation	database_states	—	alerted-state memory (#2166), persisted	not the shape

One sibling with the identical mechanism: Database File Growth's rise arm, 60-minute cadence against a 5-minute cooldown — up to twelve cards per hourly observation of one growth event. Not fixed here because the fix is not the two-line guard: the stamp is not available to the engine and would have to be plumbed through both adapters and the info type exactly as this PR does for forced plans. It should take the same observation-keyed guard in its own lane.

Tests

Executed locally (the mactest pattern — a net10.0 runner named Darling.Tests over the built Alerting/Service/Storage assemblies; Alerting is not WPF):

• AlertEngineTests: all 108 pass. Six new pins replay the production series by its clock:
  • same observation across six cooldowns → one card; the read still happens every pass; the 04:50 collection resolves it exactly as before;
  • a newer observation with a rise → a second card carrying ITS delta and total, and that observation re-read past a cooldown is one card too;
  • a new observation landing inside the cooldown fires once the cooldown elapses (the seen ≠ alerted pin);
  • a muted fire stamps the observation, so lifting the mute does not replay it;
  • recovery forgets the observation, so a later episode fires;
  • a stampless row keeps the pre-Forced Plan Failing re-fires on every cooldown against the same unchanged observation until the next collection lands: one failure, six cards #3579 cooldown-repeat.
  • Negative control: with the engine change reverted, four of the six fail (the two that pass on both are pins of preserved behaviour).
• ForcePlanFailuresAccessPathTests, all four, against a live timescale/timescaledb:2.28.1-pg18 rig (PG 18.4, TimescaleDB 2.28.1): the store migrated from V1, hypertables converted, PgTableTuning applied, six servers seeded in the collector's interleaved pattern, EXPLAIN of the shipped statement still an Index Only Scan on idx_query_store_stats_server_time_forcing with no server_id filter; the adapter returns the one risen plan with ObservedAtUtc tick-equal to the seeded newest collection_time, Kind Utc. New ungated pin: observed_at is the eighth and last column, the scan column set did not grow.
• Replayed the issue's series through the shipped SQL on the same rig by hand: between the 04:18 and 04:50 collections the read returns the seven original columns unchanged plus observed_at = the 04:18 stamp; after the 04:50 collection it returns nothing. Rig torn down.

CI is first execution (the Lite assembly is WPF and cannot load here; both test projects build -c Release -p:EnableWindowsTargeting=true with 0 warnings):

• ForcePlanFailuresSql_IsTheDarlingText_ReadingTheDedupView — Lite's text equals Darling's but for v_query_store_stats, held as whole-text equality (verified equal by script), plus the last-column pin.
• ForcePlanFailuresReadTests — the same series through Lite's real DuckDB read: the rise returned once with the newest collection's stamp to the tick, the re-read returning the same stamp, the reset collection returning nothing.

Closes #3579

[claude]

LGTM — reviewed the AlertEngine observation-guard logic (sameObservation/LastAlertedObservedAtUtc), the Darling (Postgres) and Lite (DuckDB) SQL/reader changes for byte-for-byte parity, index-coverage impact, SQL-style conventions, and edge cases (null stamp fallback, muted fire, recovery reset, cooldown-inside-window). No correctness, security, or parity issues found; the diff matches exactly what the PR description claims to touch.

[claude]

Reviewed the diff (AlertEngine.cs, ForcePlanFailureInfo.cs, both read adapters, and the Lite/Darling test suites).

What I checked:

• Correctness of the observation guard: sameObservation = observedAt <= lastAlertedAt (both non-null) is the right comparison for "fire once per new observation, not once per cooldown." Traced through all the edge cases the new tests cover — same observation across repeated cooldowns, a newer observation with a fresh rise, an observation landing inside the cooldown window, a muted fire stamping the observation, recovery clearing the memory, and a stampless row falling back to the pre-Forced Plan Failing re-fires on every cooldown against the same unchanged observation until the next collection lands: one failure, six cards #3579 cooldown-repeat. All check out against the code at AlertEngine.cs:2362-2372.
• Ordinal safety: observed_at is appended as column 7 in both DarlingAlertReadAdapter.ForcePlanFailuresSql and LocalDataService.ForcePlanFailuresSql, so the seven pre-existing ordinals (0-6) that both readers bind are untouched. Confirmed both readers use reader.GetDateTime(7) correctly.
• Index/access-path claim: verified n.collection_time was already selected from qs.collection_time pre-PR (used in the ranking/partition), so appending n.collection_time AS observed_at doesn't add a new qs. reference and the existing covering index ((server_id, collection_time DESC) INCLUDE (...)) still covers the statement without changes — the new TheObservationStamp_IsTheLastColumn... test and the pre-existing TheCoveringIndex_CarriesExactlyTheColumnsTheReadReferences both back this up.
• Lite/Darling parity: the two ForcePlanFailuresSql strings stay text-identical but for the view/table name (enforced by the new ForcePlanFailuresSql_IsTheDarlingText_ReadingTheDedupView pin), the reader mapping is identical in shape, and both adapters stamp DateTimeKind.Utc from a naive-UTC read the same way. No drift between the apps.
• SQL injection / security: both queries are fully parameterized ($1/$2), no string interpolation of external input. No concerns.
• The CI-flakiness fix in ForcePlanFailuresAccessPathTests.cs (flooring DateTime.UtcNow to whole microseconds before seeding) is correct — 10 ticks = 1 μs, matching Postgres' microsecond timestamp resolution, and explains the "three ticks" Windows CI failure cited in the comment.

No correctness bugs, parity drift, or security issues found. This is an unusually well-documented and well-tested fix (production evidence, six new AlertEngine tests, a new Lite DuckDB round-trip test, and updated access-path pins).

[claude]

LGTM — verified the observation-stamp guard logic in AlertEngine.cs (correct once-per-observation semantics with proper null/mute/recovery handling), ordinal-safe SQL changes in both read adapters, Lite/Darling parity (text-identical SQL, identical reader mapping), the covering-index claim, and parameterized queries (no injection risk). No substantive issues found.