Reach the delivery cooldown through the control plane as delivery.cooldown_minutes

[erikdarlingdata]

Fixes #3314.

email_cooldown_minutes was the only throttle between a fired alert and a Slack / Teams / PagerDuty / generic-webhook post, and it was named for email, stored in the SMTP config block, absent from get_alert_settings, refused by update_alert_settings, and writable only from the WPF Settings window. On a headless deployment with Slack configured and no SMTP at all, the sole path to the number governing channel volume was a desktop app.

get_alert_settings now reports it and update_alert_settings now accepts it as delivery.cooldown_minutes, beside the delivery.mode / delivery.per_event_max it belongs with. The stored column keeps its name and is accepted as a write-only alias, so existing configs, the Lite Settings window and any hand-written UPDATE keep working; sending both spellings in one body is refused naming both, rather than letting one win silently. Two wire keys for one setting is its own bug for a client, so only the canonical name round-trips.

The tool descriptions now say the two cooldowns are different stages — top-level cooldown_minutes gates whether AlertEngine FIRES, delivery.cooldown_minutes throttles the resulting post — because an operator reading cooldown_minutes: 5 reasonably concluded that was the delivery cadence.

Two tables, one tool

The value is on config.config_notification; every other knob is on config.config_alert_settings. That is the design decision here, and it is why the column reached 3.5.0 unreachable.

DarlingAlertReader gains a second read constant, and the partial-update parser carries a table per field instead of inferring one from the column name — an inference that would be right today and silently wrong the first time a second notification knob arrives. The write issues one statement per table in a fixed order inside one transaction: a partial application is the worst outcome available, since the caller would be told updated and handed a re-read that half-landed with no indication which half.

The round-trip invariant EveryColumnRead_IsEmittedByThePayload_AndAcceptedByTheWriter is now held per table, driven off the tool's own table list. A single equality over the union would be satisfiable by compensating drift — a column dropped from one table's write set and a stray added to the other's net to the same set — and it is weakest exactly where the new plane is thinnest. updated_fields stays unqualified, because qualifying it would redefine every existing entry of a consumer-visible array; a new test asserts no writable column name appears on both tables, which is what makes a bare name unambiguous.

Neither statement touches modified_at, matching what this tool has always done to config_alert_settings. Both Viewer upserts do bump it, and the asymmetry is checked rather than inherited: nothing reads modified_at — it is in no viewer projection, no payload and no decision — and bumping it here would mean granting mcp UPDATE on a second column of a secret-bearing table to maintain a value with no reader.

Both reads happen under one snapshot. DarlingAlertReader.GetAlertConfigurationAsync opens one connection and one transaction and returns both halves, and both tool paths go through it — get_alert_settings and the post-write re-read. Two independent reads could straddle a concurrent update_alert_settings commit and return a stale cooldown_minutes beside a fresh delivery.cooldown_minutes, which is the same half-landed state the write path takes a transaction to avoid producing, and the re-read is described to the caller as the authoritative merged state. REPEATABLE READ is the mechanism rather than a detail: PostgreSQL takes a fresh snapshot per statement under READ COMMITTED, so a default transaction around the two SELECTs would read as a fix and change nothing. The two single-table reads are private and take the combined method's connection and transaction, so the split cannot come back by calling one alone — it does not compile.

get_alert_settings reports unavailable when the notification row is absent, naming it. The service seeds both singletons in one pass, so "settings present, notification absent" is not a state the product produces, and reporting the shipped 15 would state a number nobody wrote.

The grant, measured

The mcp role gains GRANT UPDATE (email_cooldown_minutes) ON config.config_notification. This does widen mcp into a table holding bearer secrets — the SMTP password blob, the Teams/Slack/generic webhook URLs, the PagerDuty routing key — so it is column-level on that one column, the same shape as the config_service beacon grant beside it.

A missing SELECT and a missing UPDATE raise the identical 42501: permission denied for table config_notification, so the grants were isolated against a live store rather than reasoned about:

cell	result
baseline (existing non-secret column carve, no UPDATE grant)	the read returns the value; the write is 42501
+ GRANT UPDATE (email_cooldown_minutes)	the write succeeds and the value lands
UPDATE granted, the column's SELECT revoked	the read is 42501, the write still succeeds — so UPDATE is the privilege doing the work, not an ambient SELECT
SELECT restored, UPDATE revoked	the write is 42501 again

So the read side needs nothing: email_cooldown_minutes is already in the section-6 non-secret carve. Writes to smtp_encrypted_password, slack_url and even the non-secret sibling smtp_host all stay 42501, and the secret columns stay unreadable. The reload beacon is already covered — config_notification carries trg_bump_notification, which bumps config_service.config_version as the current role, and section 8 already grants mcp those two columns. Darling/tools/provision-roles.sql needs nothing: it provisions no mcp role, the network MCP endpoint being managed-mode only.

DarlingSecuritySplitLiveTests now performs that write as the mcp role and asserts the three sibling denials, so the grant's narrowness is proven rather than described.

The read cannot widen

config_notification is where #2293/#2298 played out: the MCP host's whole-row read was removed, not narrowed, because column-level denial answers for the whole table and skipping one denied column just moved the error to the next. This read is the shape that survives that — one column the carve explicitly grants. McpConfigReadAvoidsSecretColumnsTests gains the mirror of its existing pin: the privileged read must still name secrets (so the host's skip stays justified), and this MCP read must name none, with the secret set derived from the ACL so a reclassification fails on the day it happens.

The 120-minute ceiling stays, and why

DarlingAlertSettings clamps 1..120, and the MCP write bound must equal the engine clamp exactly — the parity FileGrowthWriteBounds_MatchTheEngineClamps and AgConnectionAndBlockingWaitWriteBounds_MatchTheEngineClamps already hold, for the reason that a wider bound lets the tool ACCEPT a value the engine silently rewrites on read, presenting as the setting not sticking with nothing saying no. So raising the ceiling is a behaviour change across four clamp sites in both SKUs, not a bound edit.

It should not be raised. The cooldown is one global number applied to every fingerprint on every server, while the need it is being asked to serve is one recurring signature legitimately open for hours. Stretching it silences everything else at the same cadence, including a genuinely new critical alert. create_mute_rule already does that job properly: scoped (server, metric, database, query text, wait type, job name), expiring, listed by get_mute_rules, and it still logs the alert — where a multi-hour cooldown is an unscoped, undisclosed suppression that no tool reports. update_alert_settings' description now points there.

Lite

Lite runs the same shared throttle — WebhookAlertService and EmailSendCore both hand IncidentCooldown the same value through AppAlertSettings — so Lite's get_alert_settings reports the same key. McpAlertSettingsKeyTests derives Darling's shape from source and compares both directions, so this is required for parity, not optional. Lite has no update_alert_settings; the stored spelling stays in settings.json and the Settings window.

No migration rung

The column already exists, and grants are applied by EnsureProvisionedAsync on every managed start (idempotent and self-healing, after migration) rather than by a rung. StorageVersion.SchemaVersion stays 117, so this does not contend with #3328's 118.

Verification

Darling.Tests targets net10.0-windows and cannot run on macOS, so the actual test sources were compiled into a net10.0 console harness against a live PostgreSQL 18 container: 210 passed, 0 failed, 1 skipped (the TimescaleDB-gated hypertable test). That covers DarlingMcpAlertToolsTests both classes, DarlingSecuritySplitLiveTests, McpConfigReadAvoidsSecretColumnsTests, and the whole-tree guards DocCommentHygieneTests, McpReadCommandTimeoutTests, CommandDeadlineScannerAdoptionTests, TsqlConventionGuardTests, PgReadKindDisciplineTests, StoreSqlClockDisciplineTests and ReadmeDerivedCountPinTests.

Folding the whole-tree guards in earned its keep: McpReadCommandTimeoutTests failed on the first cut. The three-argument new NpgsqlCommand(sql, connection, transaction) is the monitored-TARGET shape in its allowlist (the HypoPG experiment's, bounded by a server-side SET LOCAL), so a store command written that way reads as unclassified. Fixed by assigning the transaction rather than widening an allowlist whose whole job is to stop a target command taking a store bound.

Eleven mutations were run to prove the pins discriminate, each red in the pin that owns the property: dropping the payload key, widening the write bound to 240, naming slack_url in the MCP read, removing the mcp grant (live 42501), disabling the two-names guard, skipping the notification table in the write loop, restoring the three-argument constructor, emptying the notification SELECT list, malforming the sample-config comment, and downgrading the snapshot to READ COMMITTED. An eleventh — splitting the combined read back into two — is a compile error rather than a test failure, which is the stronger guarantee. The last one exists because the collision test compares column counts, and a count comparison is satisfied by two empty sets — so each plane is now asserted non-empty before the counts are compared. Lite's parity test cannot run here; it was checked by an independent re-derivation of the same brace walk over both payload initializers, which was itself falsified by removing Lite's key.

Two arms deliberately not covered live

The unavailable arm of the notification read, and its rollback, are not exercised by a live test. Reaching either means deleting the config_notification id=1 row, which holds the SMTP password blob, the webhook URLs and the PagerDuty routing key — so the test would be capturing and rewriting credentials, and a mid-test failure on a shared store would destroy the notification configuration. The arm mirrors the settings-row arm directly above it, which is covered, and the ordering makes a partial application impossible either way: if the settings row is missing the loop returns before touching notification, and if the notification row is missing the settings UPDATE is inside the transaction being rolled back. Covering it properly wants a dedicated throwaway database rather than a delete-and-restore against a live one; say the word and I'll add one.

Lite.Tests and the Windows suites remain CI's to arbitrate.

[claude]

Reviewed the diff (Darling C#/PostgreSQL + Lite parity; no T-SQL touched, so the T-SQL style section doesn't apply here).

Overall: this is a careful, well-tested change. The two-table write is correctly wrapped in a single transaction with a fixed statement order (avoids deadlocking against itself on concurrent calls) and rolls back cleanly on partial failure via using-disposal. The alias/canonical-name handling (email_cooldown_minutes vs delivery.cooldown_minutes) is validated per (table, column) rather than by name, and the both-spellings-sent case is explicitly refused rather than silently picking a winner — confirmed by DeliveryCooldownColumn test coverage. The permission grant is correctly scoped to column-level UPDATE on config_notification, and the accompanying live-tested sibling-column denials (smtp_encrypted_password, slack_url, smtp_host) are a good way to prove the grant's narrowness rather than assume it.

Lite/Darling parity: Lite/Mcp/McpAlertTools.cs picks up the same delivery.cooldown_minutes key and description wording, reusing the already-shared App.EmailCooldownMinutes (same 1–120 clamp). Lite has no update_alert_settings, so no write-side parity gap — matches the PR's own stated scope. Looks consistent.

One minor note (posted inline): get_alert_settings and the post-update re-read now each perform two independent, unsynchronized reads across config_alert_settings and config_notification. A concurrent update_alert_settings landing between the two reads could produce a response that mixes pre/post state across the two tables. Low real-world impact given these are near-static singleton rows, and the write path itself is fully transactional — just flagging the asymmetry since the PR is otherwise very deliberate about avoiding "half-landed" states.

No SQL injection, secret-handling, or missing-index-DMV concerns found.

[claude]

Review summary

Went through the full diff (Darling reader/tools/roles, the three test files, darling.sample.json, Darling/README.md, and Lite's McpAlertTools.cs). This is a tight, well-scoped fix and I didn't find correctness, security, or parity bugs to flag.

Specifically checked and found sound:

• Atomicity of the two-table write. UpdateAlertSettings now opens one connection/transaction, writes config_alert_settings then config_notification in a fixed order (avoids cross-call deadlock), and any 0-rows-affected result returns early — the transaction is never committed, so await using rolls it back. No half-landed state is observable.
• Read consistency. GetAlertConfigurationAsync wraps both SELECTs in one RepeatableRead transaction so get_alert_settings and the post-write re-read can't straddle a concurrent commit and report a mixed pre/post state. Good catch calling out that PostgreSQL's per-statement snapshot under READ COMMITTED would otherwise make two independent reads look consistent when they aren't.
• SQL construction. Table/column names interpolated into the UPDATE ... SET text are all compile-time constants from the switch in BuildAlertSettingsUpdate (never caller input); every value is a bound NpgsqlParameter. No injection surface.
• Two-spellings-one-column. The email_cooldown_minutes alias and canonical delivery.cooldown_minutes are both parsed to the same (Table, Column) target, and sending both is refused via a GroupBy check before any write — avoids the silent-last-write-wins failure mode and the confusing raw Postgres "multiple assignments to same column" error.
• Grant scope. The new mcp grant is column-level (GRANT UPDATE (email_cooldown_minutes) ON config.config_notification), matching the existing beacon-column pattern, and DarlingSecuritySplitLiveTests exercises both a successful narrow write and 42501 denials on secret and non-secret sibling columns (smtp_host) — confirms the grant isn't accidentally table-wide.
• Bound parity with the engine. The 1–120 clamp accepted by the tool is asserted against DarlingAlertSettings' Math.Clamp(_config.Smtp.EmailCooldownMinutes, 1, 120) by name, so the tool can't silently drift wider than what the engine will actually honor.
• Lite/Darling parity. Lite has no update_alert_settings (read-only surface), so the counterpart change there is just surfacing delivery.cooldown_minutes in get_alert_settings's payload off the existing App.EmailCooldownMinutes — correctly scoped, no missing write-side parity gap.
• Docs. darling.sample.json, Darling/README.md (both the prose and the role table), and the tool descriptions were all updated consistently with the code change.

Nothing to request changes on.