Forced Plan Failing alert — page when Query Store can't reproduce a forced plan (#2157) - #2175
Conversation
The alerting unit is one forced PLAN whose force_failure_count rose between the two most recent collections — a delta, never a level, because the counter is cumulative and travels with a restored database: level-based firing would alert forever about failures that happened on a machine the operator may not own anymore. A counter that DROPS is an unforce/re-force cycle and re-arms silently. Severity is Warning for every rise, deliberately with no Critical tier: a failing force is not an outage (the query runs on the optimizer's plan), and inventing urgency thresholds without field evidence about which reasons correlate with harm is how alert streams stop being read. The doc says what evidence would justify grading one. Model + tokens only; adapters, engine block, settings, and tests follow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds IAlertReadAdapter.GetForcePlanFailuresAsync plus both implementations in one commit, since the interface method breaks every implementor until they all have it (two adapters and four test fakes). The SQL is deliberately shape-for-shape across Postgres and DuckDB so the apps cannot disagree about what counts as a new failure: query_store_stats carries one row per plan per interval per collection with the forcing columns repeated, so each (plan, collection_time) collapses via MAX before any comparison; the newest two samples are then compared and only a RISE is returned. Equal counters are silence. A LOWER counter — an unforce/re-force reset — is silence too, not a negative delta. A plan with only one sample is omitted: 'new' is unknowable from a single observation, which costs one cycle of delay. The two-hour window bounds the scan; a plan not collected inside it is not failing now, and Query Store's 900s flush means an active plan appears several times within it. Engine block, settings gate, and tests follow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CheckForcePlanFailuresAsync follows the database-state family exactly: settings gate before the read, log-and-skip on a failed fetch (never resolve on absent evidence — that fabricates a recovery), the shared excluded-databases filter case-insensitively over ORDINAL object keys, a per-PLAN active set and cooldown, mute stamped even when muted, and a per-plan resolution when the counter stops rising. Per-PLAN keys matter: two failing plans in one database are independent conditions, and a server- or database-keyed cooldown would swallow the second one entirely. Pinned. The resolution says 'no longer failing' rather than 'fixed', because the counter also stops when the force is removed or the query stops running, and the alert cannot tell those apart. Enabled with no user-facing toggle, deliberately: the alert is silent by construction on a healthy fleet (it needs a RISE), and a darling.json-only flag would be silently reset on the first store reload since ApplyToConfig swaps the whole Alerts object — so if this ever needs to be configurable it belongs in the store. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| { | ||
| var recovered = active.Where(p => !current.ContainsKey(p)).ToList(); | ||
| foreach (var planKey in recovered) | ||
| { | ||
| active.Remove(planKey); | ||
| _lastForcePlanAlert.TryRemove(key + "|" + planKey, out _); | ||
| if (!suppressed) | ||
| { | ||
| await NotifyResolutionAsync(new AlertResolution( | ||
| key, serverName, ForcePlanTokens.MetricName, | ||
| "Forced Plan Failing Resolved", | ||
| $"{serverName}: {planKey} no longer failing to force"), ct); |
There was a problem hiding this comment.
Resolution message leaks the internal alert key instead of a human-readable identity.
var recovered = active.Where(p => !current.ContainsKey(p)).ToList();
foreach (var planKey in recovered)
{
active.Remove(planKey);
_lastForcePlanAlert.TryRemove(key + "|" + planKey, out _);
if (!suppressed)
{
await NotifyResolutionAsync(new AlertResolution(
key, serverName, ForcePlanTokens.MetricName,
"Forced Plan Failing Resolved",
$"{serverName}: {planKey} no longer failing to force"), ct);
}
}active is a HashSet<string> of ForcePlanTokens.PlanKey(...) values, i.e. strings shaped like forceplan:Sales:11:22. The recovery message interpolates that raw key directly, so the resolution an operator actually sees (in the tray toast, email, webhook, and history) reads:
SQLPROD01: forceplan:Sales:11:22 no longer failing to force
instead of something legible like SQLPROD01: Sales query 11 plan 22 no longer failing to force. Compare with the firing side a few lines up, which builds a proper human-readable string from the individual fields (failure.DatabaseName, failure.QueryId, failure.PlanId), and with CheckDatabaseStateAsync's recovery message, which this code explicitly says it mirrors — there active holds the raw database name itself, so $"{serverName}: {dbName} back to expected state" is already readable. For force-plan, the active set holds the encoded key, not the display identity, so the same pattern leaks the encoding.
ForcePlanFailure_CooldownSuppressesSecondFire_ThenResolvesWhenTheCounterStops doesn't catch this because it only asserts r.Message.Contains("22"), which is true either way.
Fix: track (database, queryId, planId) alongside the key in _activeForcePlanAlerts (e.g. ConcurrentDictionary<string, Dictionary<string, ForcePlanFailureInfo>> instead of HashSet<string>), or at minimum parse the pieces back out of planKey before building the message.
| MAX(COALESCE(qs.last_force_failure_reason, '')) AS reason | ||
| FROM v_query_store_stats AS qs | ||
| WHERE qs.server_id = $1 | ||
| AND qs.collection_time > now() - INTERVAL 2 HOUR |
There was a problem hiding this comment.
Minor: now() - INTERVAL 2 HOUR (unquoted numeral) is the only interval literal written this way anywhere in the codebase — every other DuckDB query in Lite/Services/*.cs spells this INTERVAL '2 hours' / INTERVAL '1 day' (e.g. LocalDataService.QueryStore.cs:110, LocalDataService.WaitStats.cs:627). DuckDB is generally tolerant of the bare form, so this likely runs fine, but it's worth calling out because nothing actually exercises this SQL text: LiteAlertForwardingTests.cs's fake adapter returns canned ForcePlanFailureInfo rows and never runs ForcePlanFailuresSql against a real DuckDB connection (same gap the PR description already flags for the Postgres side — "SQL-shape pins on the two adapter queries" are called out as follow-up work). If this string ever fails to parse, CheckForcePlanFailuresAsync's catch-and-log swallows it silently every sweep — Lite users would never get Forced Plan Failing alerts and nothing would signal that. Worth a quick manual run against DuckDB (or matching the quoted-interval convention) before merge, given the whole point of this alert is to catch things that otherwise fail silently.
|
Reviewed this PR (correctness, Lite/Darling parity, security, performance). Overall the design is careful and well-reasoned — rise-not-level semantics, per-plan cooldown keys, the shape-for-shape Postgres/DuckDB SQL, and the no-toggle rationale all hold up. Left two inline comments:
No SQL injection concerns (both queries are fully parameterized), no missing-index suggestions offered per repo convention, and Lite/Darling parity otherwise looks solid — settings, adapters, and test fakes were updated in lockstep on both sides. |
1. The resolution leaked the internal key — operators would have read 'forceplan:Sales:11:22 no longer failing to force' in every toast, email and history row. The active set now carries each plan's identity rather than just its key, so the recovery names the plan the way the firing message does. My test could not catch this: it asserted the message contained '22', which the leaked key satisfies. It now asserts the key prefix is ABSENT and the readable parts are present. 2. Lite's query used the unquoted INTERVAL 2 HOUR form, which appears nowhere else in the codebase. Switched to INTERVAL '2 hours', the form every other DuckDB query here already proves parses. The failure mode if it hadn't was ugly: catch-and-log every sweep, i.e. this alert failing silently — the exact thing it exists to catch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both taken, and the first one is the more embarrassing of the two:
The follow-up SQL-shape pins noted in the PR body would have caught #2 mechanically; that stays on the list. |
Review summaryWent through the diff ( Correctness
Lite/Darling parity — no drift found:
Security
Performance
Nothing blocking found. Nice test coverage on the "recovery message names the plan, not the internal key" regression. |
1 participant
What it catches
When a forced Query Store plan can't be reproduced, the query keeps running — on whatever plan the optimizer picks. Nothing in the product witnessed that: the operator's mitigation was silently not in effect, and the only trace was
force_failure_countclimbing inside Query Store.The new Forced Plan Failing alert fires per plan on a rise in that counter, carrying database, query/plan ids, MANUAL vs AUTO forcing, the engine's own failure reason, and how many failures are new.
The design decisions worth reviewing
A rise, never a level. The counter is cumulative and it travels with a restored database — restore one elsewhere and its Query Store arrives carrying every historical failure. Level-based firing would page forever about failures that happened on hardware the operator may not own anymore. So the STORE computes the delta (mirroring how the database-state adapter returns only deviating rows) and the engine never sees a level.
Corollaries, both pinned: a counter that drops is an unforce/re-force reset and is silence, not a negative delta. A plan with only one sample is omitted — one cycle of delay, because "new" isn't knowable from a single observation, and firing on first sight is exactly how you'd alert on every restored database's imported history.
Warning for every rise, no Critical tier. A failing force isn't an outage. Grading one Critical would need evidence about which reasons or rates correlate with harm, and I don't have it; the token doc says what evidence would justify it later.
Per-PLAN keys. Two failing plans in one database are independent conditions that resolve independently — a server- or database-keyed cooldown would swallow the second entirely. Pinned by a test with two plans in one database.
The resolution says "no longer failing", not "fixed", because the counter also stops when the force is removed or the query stops running, and the alert can't distinguish those.
Enabled with no toggle. The alert is silent by construction on a healthy fleet (it needs a rise), so there's nothing for a switch to protect anyone from. Notably a
darling.json-only flag would NOT have worked: the store reload swaps the wholeAlertsobject, so it would silently reset on the firstconfig_versionbump — if this ever needs to be configurable it belongs in the store, with the ladder that implies.Field motivation
2026-08-10 tenant migrations: six databases restored onto new hosts arrived carrying 51 forced plans between them. Verifying they still reproduced took three hand-run baseline/delta passes across the wave. This alert is that check on a collection cadence.
Testing
AlertEngineTests: the disabled gate skips the READ (not just the fire), two plans in one database both alert with reason/forcing/delta in the body, cooldown suppresses the second fire and a per-plan resolution follows when the counter stops, and an excluded database never alerts (case-insensitive, parity with the other database-scoped families). FourIAlertReadAdapterfakes and fourIAlertEngineSettingsfakes updated; Alerting, Darling service, Lite, and both test projects build clean.Store SQL is shape-for-shape across Postgres and DuckDB so the apps can't disagree about what counts as new. Remaining for a follow-up: SQL-shape pins on the two adapter queries (the delta comparison, the forced filter, the two-hour bound).
Fixes #2157.