Add the custom-alert core: rule store, persistence gate, evaluator (#3285)

[erikdarlingdata]

First implementation slice of custom alerting (#3285): the rule store, the shared persistence gate, and the evaluator. Backend only — rules are authored via the store API in this slice; the MCP tools, the web editor / "alert from panel", the PostgreSQL catalog measures, and starter templates are the following slices.

What's in

• V116 migration — config.custom_alert_rules + config.custom_alert_state; SchemaVersion 116; viewer schema-probe sentinel + top-rung arm.
• AlertPersistenceGate (PerformanceMonitor.Alerting) — the shared fire-after-N / resolve-after-M hysteresis primitive (the No alert requires its condition to PERSIST: real fire/resolve pairs 55-87 seconds apart on both engines — and it is not the re-fire class closed seven times #3282 persistence contract), pure + 12 unit tests. The built-ins adopt the same gate as No alert requires its condition to PERSIST: real fire/resolve pairs 55-87 seconds apart on both engines — and it is not the re-fire class closed seven times #3282's own work.
• Rule/state stores + definition validator — CustomAlertRuleStore / CustomAlertStateStore (owner pool), CustomAlertRuleDefinition validated by the compose TryParsePanel authority and pinned to Scalar (15 parser tests).
• CustomAlertEvaluator — compile → run on a dedicated viewer-role pool (which carries the statement_timeout cap and the least-privilege ACL, never the owner/superuser pool) → predicate → persistence gate → deliver/resolve via DarlingAlertDeliverer, keyed on metric_name="Custom:<rule_id>" (rename-safe). No-data freezes the streak; runs above the connect gate so a rule still evaluates for a disconnected server.
• Role grant — viewer+mcp INSERT/UPDATE/DELETE on config.custom_alert_rules (the state table stays owner-only).

Verification

• Full solution Rebuild: 0 Warning(s) / 0 Error(s).
• 75+ tests green: the persistence gate, the definition parser, the role-grant shape pins, and the schema-version probe pins. No live-DB test in CI yet (see below).

Scope boundaries (tracked on #3285)

Deferred to later slices: MCP CRUD tools, web editor + "alert from panel", PG catalog measures, starter templates, severity-escalation re-delivery, tag scope, count with </<= (validator rejects it — dead-collector footgun), the broken-rule self-health alert (currently logs a warning), disable/delete orphan-reconcile, display-name in notification titles (the body carries the rule name; titles still show Custom:<id>), and BYO / non-Windows viewer-pool wiring (the evaluator only starts on a managed Windows store).

Not in this PR

• Live end-to-end on DARLING01 (applying V116 to the production store + running against the live fleet) — held for explicit go.
• CHANGELOG entry — deferred to the batch splice per the parallel-wave protocol.

🤖 Generated with Claude Code

https://claude.ai/code/session_01WejwpgWF5Xfm3fAmoEbFz4

[claude]

Likely runtime bug: unstripped DateTimeKind bound to a timestamp (no-tz) column.

state.LastEvaluatedAt / state.NextDueAt originate from DateTime.UtcNow in CustomAlertEvaluator (Kind = Utc), and are bound here straight through with NpgsqlDbType.Timestamp. custom_alert_state.last_evaluated_at / next_due_at are declared timestamp (naive) in the V116 migration.

CONTRIBUTING.md calls this out explicitly under "Darling's PostgreSQL store":

Strip the DateTimeKind before binding a timestamp parameter — DateTime.SpecifyKind(value, DateTimeKind.Unspecified). Npgsql infers timestamptz from a Utc or Local kind... the row lands at the wrong hour with no error anywhere.

With Npgsql 10 (this repo's pinned version), binding a Kind=Utc DateTime to an explicit NpgsqlDbType.Timestamp parameter throws InvalidCastException at execute time (Cannot write DateTime with Kind=Utc to PostgreSQL type 'timestamp without time zone') — there's no global Npgsql.EnableLegacyTimestampBehavior switch anywhere in this repo, and every other store in Darling (PgCollectorRowWriter.Naive, QueryStorePlanMap.Naive, PgStatementText.Naive, ViewerDataService.MuteRules.Naive, etc.) strips the Kind first for exactly this reason.

If this throws, SaveAsync fails on every evaluation after DeliverFireAsync/DeliverResolveAsync already ran (see CustomAlertEvaluator.EvaluateRuleForServerAsync), the exception is swallowed by the per-rule catch in EvaluateServerAsync, and the state row never persists — so LoadAsync keeps returning Fresh every sweep. For a breachSamples = 1 rule that means the fire notification re-sends every ~60s sweep forever instead of firing once and resolving normally; for breachSamples > 1 it means the rule can never actually reach threshold. This path has no live-DB test in this PR, which is likely why it wasn't caught.

Suggest applying the same fix as elsewhere: Value = (object?)(value.HasValue ? DateTime.SpecifyKind(value.Value, DateTimeKind.Unspecified) : (object?)null) ?? DBNull.Value.

[claude]

This comment says config.custom_alert_state "deliberately gets NO viewer/mcp grant here", but that's not actually true given how provisioning works: section 2 above (GRANT SELECT ON ALL TABLES IN SCHEMA {config} TO {admin}, {viewer}; and the {mcp} twin in section 6) is a blanket, re-run-every-start grant over every table in config, and custom_alert_state has no entry in ViewerRestrictedConfigTables (that mechanism only strips/re-grants column-level access for the three secret-holding tables). So after V116 creates custom_alert_state and provisioning re-runs, viewer and mcp already have full-table SELECT on it — the state/firing data isn't actually gated behind "when the editor surfaces a rule's firing status."

Not a security-critical leak (firing state isn't secret), but the comment is asserting an access boundary that doesn't exist, which could mislead whoever later adds the editor's read path into thinking they need to add a grant that's already implicitly there — or, if hiding this table's contents from viewer/mcp was actually intended, it needs an explicit carve (like ViewerRestrictedConfigTables) rather than just omitting the INSERT/UPDATE/DELETE grant.

[claude]

Reviewed the custom-alert core slice (rule store, persistence gate, evaluator). This is Darling-only by design (Postgres/viewer-pool wiring; Lite has no analogous custom-rule surface yet), and that's explicitly scoped out in the PR description, so no parity flag there.

Findings posted inline:

1. Likely runtime bug (CustomAlertStateStore.cs, AddNullableTimestamp): LastEvaluatedAt/NextDueAt (both DateTime.UtcNow-derived, Kind = Utc) are bound straight through to a NpgsqlDbType.Timestamp parameter without stripping the DateTimeKind, contrary to the convention CONTRIBUTING.md documents and that every other Darling store follows (Naive()/AsNaive() helpers throughout). With Npgsql 10 this throws on SaveAsync, which — because the exception lands after delivery and is swallowed by the per-rule catch in CustomAlertEvaluator — means state never persists and a firing rule likely re-fires every sweep instead of firing once and resolving normally. There's no live-DB test in this PR exercising SaveAsync, which is probably why it wasn't caught.

2. Misleading grant comment (DarlingManagedRoles.cs): the comment claims config.custom_alert_state "deliberately gets NO viewer/mcp grant," but the blanket GRANT SELECT ON ALL TABLES IN SCHEMA config (re-run every provisioning start) already gives viewer/mcp full read access to it — there's no ViewerRestrictedConfigTables-style carve for this table. Not a secret-exposure issue, but the stated access boundary doesn't match reality.

Everything else — the persistence-gate semantics (fire/resolve edges, streak capping, non-positive sample clamping), the rule-definition validator (scalar-only, count+</<= rejection, critical-vs-warn ordering, window ceiling), the store's parameterization (all bound $N params, no string-built SQL), and the migration ladder bookkeeping (rung/version/pins/viewer-probe arm) — looked correct and consistent with existing patterns in the codebase.

🤖 Generated with Claude Code

[claude]

Bug: SaveAsync will throw on essentially every call — custom-alert state never persists.

value here is state.LastEvaluatedAt / state.NextDueAt, which in CustomAlertEvaluator.EvaluateRuleForServerAsync are derived from DateTime.UtcNow (var now = DateTime.UtcNow; in EvaluateServerAsync, then nextDue = now.AddSeconds(...)) — both have DateTimeKind.Utc. They're bound here with NpgsqlDbType = NpgsqlDbType.Timestamp (i.e. PG timestamp without time zone) without stripping the Kind.

Npgsql 10 (this repo pins Npgsql 10.0.3) rejects writing a Utc-kind DateTime to a timestamp column — it throws InvalidCastException at ExecuteNonQueryAsync, and there's no Npgsql.EnableLegacyTimestampBehavior switch set anywhere in the repo to relax that. Every other store in this codebase strips the kind first (PgAlertStateStore.Naive(), PgMuteRuleStore.Naive(), the DateTime.SpecifyKind(x, DateTimeKind.Unspecified) calls throughout DarlingCollectorRunner/DarlingObservability/etc.), and CONTRIBUTING.md calls this out explicitly under "Darling's PostgreSQL store":

Strip the DateTimeKind before binding a timestamp parameter — DateTime.SpecifyKind(value, DateTimeKind.Unspecified).

Net effect: CustomAlertStateStore.SaveAsync throws every time LastEvaluatedAt/NextDueAt is non-null (i.e. every real evaluation). It's swallowed by the per-rule catch (Exception ex) in CustomAlertEvaluator.EvaluateServerAsync, so the service won't crash, but the persistence gate's counters/firing/next_due_at never actually get written — the rule re-evaluates from Fresh state on every sweep and the feature is non-functional against a live store. This is almost certainly why it wasn't caught: the PR has no live-DB test exercising this store (CustomAlertStateStore/CustomAlertEvaluator aren't referenced anywhere in Darling.Tests), and the PR description notes "No live-DB test in CI yet."

Fix:

Suggested change

	private static void AddNullableTimestamp(NpgsqlCommand command, DateTime? value) =>
	command.Parameters.Add(new NpgsqlParameter
	{
	NpgsqlDbType = NpgsqlDbType.Timestamp,
	Value = (object?)value ?? DBNull.Value,
	});
	private static void AddNullableTimestamp(NpgsqlCommand command, DateTime? value) =>
	command.Parameters.Add(new NpgsqlParameter
	{
	NpgsqlDbType = NpgsqlDbType.Timestamp,
	Value = value is DateTime dt ? DateTime.SpecifyKind(dt, DateTimeKind.Unspecified) : (object)DBNull.Value,
	});

[claude]

Minor, secondary to the write-side bug below: reader.GetDateTime(5)/(6) reads these timestamp (naive) columns back as DateTimeKind.Unspecified, but every other store that round-trips a naive-UTC column re-stamps it on read (DateTime.SpecifyKind(reader.GetDateTime(...), DateTimeKind.Utc) — see PgMuteRuleStore.CreatedAtUtc, PgAlertStateStore, DarlingSelfAlertEvaluator, etc.). Doesn't cause a functional bug today (DateTime comparisons ignore Kind), but worth doing for consistency once the write side is fixed, since these values do get compared against DateTime.UtcNow in CustomAlertEvaluator.

[claude]

Review summary

Solid first slice — the AlertPersistenceGate primitive is cleanly separated (pure, well-tested edge cases including overflow-clamping and degenerate sample counts), the rule validator reuses the compose TryParsePanel authority instead of re-validating metrics, the migration rung follows the established V31/custom_views pattern (schema-qualified, no bump trigger, sentinel wired into MapProbedSchemaVersion above the previous top rung), and the evaluator is properly failure-isolated per rule.

One correctness bug that looks blocking, left as an inline comment: CustomAlertStateStore.AddNullableTimestamp binds state.LastEvaluatedAt/NextDueAt (both DateTimeKind.Utc, from DateTime.UtcNow) to a NpgsqlDbType.Timestamp parameter without stripping the Kind first. Npgsql 10 (pinned in this repo) throws InvalidCastException writing a Utc-kind DateTime to a naive timestamp column, and there's no legacy-timestamp switch set anywhere in the repo to relax that. Every other store here does DateTime.SpecifyKind(value, DateTimeKind.Unspecified) first (PgAlertStateStore.Naive(), PgMuteRuleStore.Naive(), etc.), and CONTRIBUTING.md calls this exact landmine out under "Darling's PostgreSQL store." Net effect: CustomAlertStateStore.SaveAsync throws on essentially every real evaluation, gets swallowed by the per-rule catch in CustomAlertEvaluator.EvaluateServerAsync, and the feature silently never persists state against a live store. Consistent with the PR's own "No live-DB test in CI yet" caveat — neither CustomAlertStateStore nor CustomAlertEvaluator is referenced from Darling.Tests, so nothing currently exercises this write path.

Lite/Darling parity: no gap here — custom_views (the pattern this PR explicitly mirrors) is itself Darling-only with no Lite counterpart, so a Darling-only migration rung + store for a centrally-authored, web-editor-surfaced entity is consistent with existing precedent, not a drift from it.

No other correctness, security, or performance issues found — SQL is fully parameterized throughout, migration is properly schema-qualified, and the severity-escalation / disable-orphan-reconcile gaps called out in the PR description as deferred scope are indeed handled as "no-op" rather than silently wrong.

🤖 Generated with Claude Code