Skip to content

Per-server threshold overrides: loader, PostgreSQL counts, set_alert_threshold_override (#5916) - #5933

Merged
erikdarlingdata merged 9 commits into
devfrom
fix/5916-threshold-overrides
Oct 16, 2026
Merged

erikdarlingdata merged 9 commits into
devfrom
fix/5916-threshold-overrides

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Oct 16, 2026 •

Copy link
Copy Markdown
Owner

Claude posting for Erik Darling.

Fixes #5916.

Part of #5632, Phase B wave 3. This lands all four parts of the issue: the loader and engine sites, the tool and view, the README section, and the two grids (part 4, see ## Part 4: the two grids).

Why

Some alerts fire often on one server and not on others. Darling can now hold a threshold for one server (or one database) that differs from the global value, and a person or a Level 2 agent can set it. The global row is untouched.

What changes

  1. Loader. DarlingThresholdOverrides (new, Darling/PerformanceMonitor.Darling.Service/DarlingThresholdOverrides.cs) implements IAlertThresholdOverrides from config.config_threshold_overrides. It holds one immutable in-memory snapshot (no I/O on the sweep path), loads at start and on every control-plane reload (the table is on the config_version beacon), and a failed load keeps the snapshot already in force. A stored row that is not a tunable key, not an integer, out of bounds, or a database name on a server-only key is skipped and logged, never applied. A database override wins for that database; a server override covers the server's other databases. DarlingThresholdKeys is the one key table: every The alert engine takes per-server and per-database threshold overrides #5910 registry key plus the two PostgreSQL count thresholds, named as get_alert_settings names them (cpu.threshold_percent, blocking.pg_count_threshold, ...), with the same bounds update_alert_settings and the Viewer enforce. PostgreSQL predictor constants have no entry.
  2. Resolution sites. DarlingWorker.BuildAlertEngine passes the loader to AlertEngine. The two PostgreSQL count checks (EvaluatePgDeadlocksAsync, EvaluatePgBlockingAsync) resolve their bar through ResolveWire with the snapshot's server key. Lite passes nothing and is untouched (no Lite write path).
  3. Tool and view. set_alert_threshold_override (server_name, key, value, database_name, clear, set_by, agent_action), in Mcp/DarlingMcpThresholdOverrideTools.cs. It is in WriteTools and AgentActionTools, so both arms offer it. Writes arm: an ordinary write, agent_action ignored, row set_by is the set_by argument (default mcp). Investigations arm: agent_action required, the change goes through DarlingAgentActionGate (request's server only, journaled action_called then action_done), the row records agent:<agent_id> and the request id, and a kind rule refuses the value that turns the alert off (each key's top of range, or zero for the four low_disk.* keys; use a mute). Details under ## Review round 1. Refused with {status: refused, reason}: key outside the registry (not_in_registry), self_alerts.* / enable / cooldown / route keys (not_tunable), out of bounds, a database name on a server-only key. Without the mcp role's grant (bring-your-own store before provision-roles.sql is re-run) it answers {status: precondition} naming the script. get_alert_settings takes an optional server_name and adds server_view: each threshold's global value, effective value and source (global or server_override), plus the server's database overrides. The web /api/read/get_alert_settings passes the same server_name.
  4. Guide. Darling/README.md, section "Per-server threshold overrides".

Test plan

  • ThresholdOverrideTests (no store, 22 cases): refused key classes (outside the registry, self_alerts.*, enable, cooldown, route, empty), out-of-bounds values, database scope only on database-scoped keys, an override moves its server and leaves another alone, a database override applies to that database only, bad stored rows are not applied, the loader moves one server's alert through AlertEngine and leaves the other's, each PostgreSQL count threshold follows its server's override through RollingCountAlertGate, the agent kind rule, and the tool offered by the right arms.
  • ThresholdOverrideLiveTests (11 cases, live PostgreSQL rig): writes arm sets, updates and clears and ignores a garbage agent_action, the loader reads what the tool wrote (both PostgreSQL keys and a database row), each refused class leaves the table empty, investigations arm requires agent_action / records the agent and request / refuses another server / refuses a disabling value, a role with no grant gets precondition naming provision-roles.sql, get_alert_settings with server_name shows effective value and source. 11 of 11 passed on a throwaway rig; the full run of both classes passed (33 of 33 for *ThresholdOverrideTests*, which also matches the T1 class).
  • Pins updated deliberately for one added tool (the budget, write-policy, classification, DI-seat and web-catalog pins): *Mcp*, *ToolsList*, *WebEndpoints*, *AdminPage*, *Census*, *Ratchet* classes: 2888 run, 0 failed after the updates (6 pins moved: tool counts 188 to 189 and 93 to 94 Darling-only in the instructions census, get_alert_settings now takes server_name, DarlingMcpServedPolicy added to the DI-seat pin, set_alert_threshold_override added to the web excluded-tools pin, and the investigations-arm annotation check now allows the Level 2 action tools).
  • tools/list budget: the tool adds one head and one guide; McpToolsListBudgetTests (head, guide and parameter-description caps) passes. get_alert_settings gained one parameter whose description is under the 200-character cap. Served counts: default 163 to 164, catalog 188 to 189 (the instructions census now reads "exposes 189 tools ... remaining 94 are unique to Darling"). I did not take a separate token measurement beyond the passing budget tests.
  • Full suite not run on the laptop; CI decides.
  • Web UI smoke: see ## UI smoke. Not run (the smoke check is red on every PR for an unrelated reason, Darling web UI smoke is red on every PR since 06:00Z on 2026-10-16: the four page baselines no longer match dev #5929; no baseline touched).

Part 4: the two grids

Admin login only. Both grids list every override with its server, database, key, value, the global value it replaces, who set it, the request id and when.

  • Viewer. ThresholdOverridesWindow (own XAML and code-behind) opened from Settings, Alert rules, "Manage Threshold Overrides...", beside the global thresholds. Its logic is ThresholdOverridesViewModel (no WPF): Save checks the server, the key (DarlingThresholdKeys.RefusalFor), a whole-number value and the bounds and database scope (ValidateValue) before it calls the store, so a refused entry never reaches a write; Remove needs a row. A read-only seat (connectAs = "viewer") gets the grid and no form, and a store refusal becomes a sentence (the read-only role's refusal in the view-model, any other store failure in the window's own Save and Remove handlers, after review round 1). The store seam is IThresholdOverrideGateway, which ViewerDataService.ThresholdOverrides.cs implements.
  • Web. GET /api/threshold-overrides (the list, the key table with bounds, the registry's server names, can_edit) and PUT /api/threshold-overrides (set, or clear: true), mapped in DarlingWebEndpoints.cs (WebRouteStatementCensusTests allows no other mapper file; every body goes through the statement sweep). The write is DarlingMcpThresholdOverrideTools.SetAsync on the writes arm, so the key registry, bounds, database scope and the statement are the tool's own; set_by is the seat's editor name shaped to the store's rule. Answers: 200 ok or cleared; 400 for a refused key, value, database, server or body; 415 without application/json; 403 with the admin-login sentence (DarlingWebThresholdOverrides.RefusedMessage) when the store login holds no write grant, which is the viewer role on managed and compose stores (the tab is then list-only and its note says so, and the Change column and form are not drawn).
  • Admin page tab. admin.js gains a sixth tab (the page has six), "Threshold Overrides" (buildOverrides, its own pure exports overrideRows, validateOverride, overrideOutcome, overrideColumns). The form and the Edit and Remove buttons show only when both the session says the seat can write and the list says the store login holds the write grant.
  • Shared code moved so the Viewer can reach it. DarlingThresholdKeys moved from the Service project to PerformanceMonitor.Alerting (public; the Viewer references Alerting but not the Service) and gained each key's config_alert_settings column, used for the global value. The store statements (ReadValueAsync, UpsertAsync, DeleteAsync, the list and the global read) moved into Darling.Storage as ThresholdOverrideStore, which the tool, the web route and the Viewer all call. No behaviour change to the tool.
  • Pin. ReadAdmissionCoverageCensusTests extended with the two routes (GET as a definition read, PUT as a write), as Investigation notes: approve or reject pending agent notes in the viewer (#5903) #5908 and People can give feedback on investigation reports: store, web tab, false-positive counts (#5913) #5926 did. No new census.
  • README. The "Per-server threshold overrides" section gains "The grids".

Part 4 tests

  • ThresholdOverrideWebLiveTests (3, live PostgreSQL rig, new): as the managed admin role the list shows the override with its global value (80), servers and keys, and set, change, database row and clear are accepted and stamped; a bad key, self_alerts key, out-of-bounds, database on a server-only key, missing value, non-integer value, missing server, non-object body and wrong content type are refused (400 or 415) with the table unchanged; as the managed viewer role the list works and can_edit is false, and three changes (set, change, clear) each answer 403 with the admin-login sentence and leave the stored value alone.
  • ThresholdOverridesViewModelTests (18, no store, new): 14 refused entries (no server, key outside the registry, empty, self_alerts, enable, cooldown, below, above and negative bounds, non-numeric, empty and fractional values, database on a server-only key, long_running_job.multiplier 21) each say why and make zero store calls; a good entry is written once per call with the login shaped to the store's rule; every key the form offers accepts its own min and max; Remove needs a row; a read-only seat gets a sentence.
  • ThresholdOverrideAdminTabTests (11, Node, new): rows, form refusals, answer wording (a 403 carries the admin-login sentence through), and the Change column only for a seat that can write.
  • Targeted run on the rig (UTC, shared_preload_libraries = 'timescaledb,pg_stat_statements'): ThresholdOverrideTests 25, ThresholdOverrideLiveTests 11, the three new classes above, plus *WebEndpoints*, *AdminPage*, *Admin*, *WebRoute*, *Census*, *Ratchet*, *Mcp*, *ToolsList*: 3300 run, 0 failed. Build 0 warnings. The first run caught WebRouteStatementCensusTests (routes belong in DarlingWebEndpoints.cs, and a handler named WriteAsync reads as a direct response write); fixed. Rig stopped by pg_ctl stop.
  • Full suite not run on the laptop; CI decides. The Viewer window was built but not opened by hand (no desktop session in the lane); the view-model carries the tested logic.

UI smoke

The web Admin page (#/admin, the admin-servers baseline page) now draws the sixth tab, "Threshold Overrides", in its tab bar, so that page's rendering changes by one tab button. The smoke check was not run and no baseline under tools/ui-smoke/baselines was touched (the check is red on every PR for #5929); the baseline for admin-servers may need the new tab link when #5929 is fixed.

What the coordinator should double-check

  • The agent kind rule now refuses each key's off end (see ## Review round 1). Check the two judgment calls there: the File Growth look-back window has no off end, and the Failed Job look-back's off end is its shortest window (1 minute).
  • DarlingMcpServedPolicy is a new DI service parameter (the policy the host started with) so one tool can tell its two arms apart; Level 2: agents change mute rules and custom alert rules, journaled #5915 will need the same seat and may want to share it.
  • Override keys are the wire names get_alert_settings uses (for example cpu.threshold_percent), not the enum names.

Review round 1

Claude posting for Erik Darling. Fixes for the review of 97f7990, one push. Targeted run on a throwaway UTC rig (timescaledb, pg_stat_statements): *ThresholdOverride*, *AlertReadFailure*, *McpPayload*, *McpServiceParameterDiSeat*, *ComposeQueryStoreStampGuardLiveTests: Total: 175, Failed: 0. The wider run (*Mcp*, *Tool*, *Description*, *Instruction*, *Alert*, *Viewer*, *Web*, *Guard*, *Admin*, *Hygiene*, *Slow*, plus *Census*, *Ratchet*, *Pin* earlier) ran 11355 tests with one failure, ComposeQueryStoreStampGuardLiveTests.TheOneDayFleetPanel_..., which passes on its own re-run and touches nothing here. Lite.Tests: CrossAppMcpToolInventoryPinTests, CrossSkuSurfaceSourceTests, *Census*, *Pin*, *AlertReadFailure*, *McpToolsListBudget*: Total: 350, Failed: 0. Both projects build with 0 warnings. origin/dev was merged (clean). No full suite on the laptop; CI decides.

  1. Blocker 1, kind rule (DarlingMcpThresholdOverrideTools.cs AgentKindRule, DarlingThresholdKeys.cs Entry.OffEnd). One per-key definition: each Entry carries an OffEnd, and the rule refuses a value AT it for every key. No caps, no bands. Off ends: the top of the range for the counts, minutes, percent and size keys (cpu and tempdb 100, pvs.threshold_percent and file_growth.volume_percent 100, long_running_job.multiplier 20, the rest int.MaxValue); zero for the four low_disk.* keys. Two judgment calls, flagged for you: file_growth.lookback_minutes has NO off end (it is the window the growth rate is averaged over, so no value turns the alert off), and failed_job.lookback_minutes has its SHORTEST window (1) as the off end, not 1440, because a longer look-back finds more failed jobs, not fewer. A very high count below the top (blocking.count_threshold = 2147483646) is allowed; it still fires if that count is reached. The tool Description and the README bullet ("Setting one") now say exactly this. Test: ThresholdOverrideLiveTests.AnAgent_IsRefusedEachKeysOffEndValue_AndStillMayUseOneBelowIt calls the tool as a Level 2 agent with a hand-written off-end value for each of the 22 keys that have one and expects threshold_disables_alert each time, with the table still empty, then shows int.MaxValue - 1 and the File Growth window at 1440 are accepted. The values are written in the test, not read from the registry, so a wrong registry value does not agree with itself.
  2. Blocker 2, unguarded async void (ThresholdOverridesWindow.xaml.cs Save_Click, Remove_Click). Both wrapped with catch (Exception ex) when (ex is not OperationCanceledException) then SetStatus(..., true), as LoadAsync and NotificationRoutesWindow do. The "never as an exception to the window" sentence is in the ThresholdOverridesViewModel.cs class comment (line 59), not the window's; it now says the view-model turns a read-only refusal into a sentence and the window's handlers catch the rest. Not unit tested: the window is WPF code-behind and the lane has no desktop session, so I did not open it. The earlier claim "a store refusal becomes a sentence" is true for the window only now.
  3. Duplicated global-value mapping (DarlingMcpThresholdOverrideTools.cs GlobalValueAsEngineReadsIt and Evaluated; DarlingMcpAlertTools.cs payload). server_view and the enclosing payload now read the same function, and the pair clamp (RunnableQueueEvaluator.ClampBars, AttentionEvaluator.ClampBars) is applied to the effective value too, so an override that inverts a warning/critical pair shows the pair the engine evaluates. Test: ThresholdOverrideLiveTests.ServerView_ShowsTheWarningAndCriticalPairTheEngineEvaluates_NotTheStoredOne. Not aliased to AlertThresholdKeys.GlobalValue itself: that switch takes an IAlertEngineSettings (54 members) and the tool holds an AlertSettingsReadRow, so an adapter implementing the interface would be a new copy. The row-side switch now exists once and both consumers call it. Entry.Column stays: it names a SQL column, not a value mapping, and the grid shows the stored global value (valid by construction). Coordinator: say if you want the adapter anyway.
  4. Smaller note 1, "fifth tab" is the sixth: corrected in ## Part 4 and ## UI smoke. The review adds that the baseline gains two tab links against the pre-People can give feedback on investigation reports: store, web tab, false-positive counts (#5913) #5926 baseline; this PR's own share is one tab button.
  5. Smaller note 2, bare table aliases (ThresholdOverrideStore.cs ListSql, GlobalRowSql): AS added.
  6. Smaller note 3, FK failure on a server removed from config but not yet mirrored (DarlingMcpThresholdOverrideTools.cs, the ForeignKeyViolation catch): the tool answers {status: refused, reason: server_not_registered} with a sentence, and the web route turns that into its normal 400 with the message. This catch and the existing precondition catch return a status word, not FormatError, so McpPayloadContractCensusTests.AdHocErrorReturns has one deliberate entry for this tool, with the reason in its comment.
  7. Smaller note 4, Replace skips silently (DarlingThresholdOverrides.cs): Replace now logs a skipped row through the same LogSkipped as LoadAsync.
  8. Smaller note 5, previous_override truncates (DarlingMcpThresholdOverrideTools.cs): the stored double is reported as it is.
  9. Also from the review's inline notes. DarlingWorker.cs: LoadThresholdOverridesAsync now sits after LoadMuteRulesAsync, so the mute-rule doc block is attached to its own method again. DarlingWebThresholdOverrides.cs RefusedMessage: the 403 sentence now also says that an admin login on a store that has not had provision-roles.sql re-run needs the script (the route cannot tell the two causes apart). The Admin tab's note "an override moves a threshold and never turns an alert off" (admin.js) now reads "To silence an alert, mute it.", since a person is not bound by the kind rule.
  10. CI red, Lite pins and what they exposed. CrossAppMcpToolInventoryPinTests: the new tool's attribute used Name = ToolName, which the parity scan cannot read; it is now the literal "set_alert_threshold_override" (a source fix, not a pin edit). CrossSkuSurfaceSourceTests.TheFleetScopedInventory_MatchesWhatTheCounterActuallyRecords: 14 to 15 (the threshold-override reload records a fleet-scoped read failure), with the name added to AlertReadFailureCounter.FleetScopedReads (shared prose, so both SKUs' get_collection_health descriptions now name it) and to the test's asserts. Running the Darling classes that read the same sources turned up two more pins the same change shifted, updated with reasons: AlertReadFailureSurfaceTests (CountedSites 43 to 44, LoadThresholdOverridesAsync added to the scanned worker members, WorkerCountedSites 12 to 13) and McpPayloadContractCensusTests (the ad-hoc roster entry above).
  11. Refuted claims corrected: six tabs; the store-refusal sentence for the window (see item 2).

UI smoke (round 1)

tools/ui-smoke baselines were not re-rendered. The admin-servers baseline shifts by one tab button, "Threshold Overrides". Not run.

CHANGELOG entry

SECTION: Added
ENTRY: - Per-server alert threshold overrides ([#5933]) - Darling can hold a threshold for one server, or one database on it, that differs from the global value, so an alert that is noisy on one server can be tuned without changing every other server. It covers the integer alert thresholds and the PostgreSQL deadlock and blocking counts; set one with set_alert_threshold_override, and see the effective value and its source with get_alert_settings and server_name. The desktop Viewer (Settings, Alert rules) and the web Admin page (Threshold Overrides tab) show a grid of the overrides with the global value beside each; both refuse a bad key or bound, and the web accepts a change only under an admin store login (a managed or compose store connects the web dashboard read-only, so there it is a list). A Level 2 agent may change an override only for its own request's server and can never set a threshold to the value that turns its alert off (the top of the key's range, or zero for Low Disk; to silence an alert, mute it), and a bring-your-own store needs provision-roles.sql re-run once to grant the table.
REF: [#5933]: #5933

Dev 67835f0 (#5926, the Report Feedback tab) was merged into this branch. The only conflict was wwwroot/js/pages/admin.js: the header comment now says "Six tabs" and lists Servers, Notification Routes, Alert Settings, Report Feedback, Pending Notes and Threshold Overrides, and the builder map carries all six (feedback: buildFeedback from dev and overrides: buildOverrides from this PR); node --check passes. ReadAdmissionCoverageCensusTests.cs pins both PRs' routes, and DarlingWebEndpoints.cs and Darling/README.md auto-merged with both PRs' routes and sections present. Darling.Tests built with 0 warnings. On a throwaway PG rig (stopped afterward), the classes *AdminPage*, *Admin*, *WebEndpoints*, *WebRoute*, ThresholdOverride*, *Census* and *Ratchet* ran with Total: 1140, Failed: 0, Skipped: 0. The Node harness tests for admin.js (ThresholdOverrideAdminTabTests, AdminReportFeedbackBehaviourTests and the other *Admin* classes) are in that run. No full suite ran locally; CI decides.

Merge of dev (#5937)

Claude posting for Erik Darling.

dev (#5937, practice requests) was merged into this branch in one merge commit. Two files conflicted, both resolved by keeping both sides:

Auto-merged files checked: ReadAdmissionCoverageCensusTests.cs has both routes, DarlingWebEndpoints.cs, DarlingMcpHostService.cs and Darling/README.md have both sides with no duplicate heading.

No other pin moved: the tools/list budget, route census and read-admission census passed unchanged, so no count was edited.

tools/ui-smoke baselines were not re-rendered. The admin-servers baseline now shifts by two tab buttons (Practice Scores and Threshold Overrides over the dev baseline), so it will differ until re-rendered.

Test run on the merged tree (throwaway PostgreSQL rig from pg-runtime.zip, stopped afterwards):

  • dotnet build of Darling.Tests and of the Darling Viewer: 0 warnings, 0 errors. node --check admin.js: OK.
  • Classes *Census* (includes SlowClassCensusTests, McpServiceParameterDiSeatCensusTests, ReadAdmissionCoverageCensusTests), *Ratchet*, *Pin*, McpToolsListBudgetTests, DarlingMcpWritePolicyTests, *ThresholdOverride* (live and Node): Total 1235, Failed 0, Skipped 2 (the two DarlingPgRuntimeVersionPinTests _Gated tests, which need DARLING_TEST_PGRUNTIME).
  • admin.js Node classes (AdminPage*, AdminServerEdit*, AwsRoleEdit*, ThresholdOverrideAdminTabTests) plus InvestigationPractice* and ReportFeedback*: Total 437, Failed 0.
  • Not run: the full suite (CI decides).

Merge of dev (#5934)

Claude posting for Erik Darling.

Merged origin/dev (Level 2 mute and custom-rule tools, #5934) into this branch as one merge commit.

Conflicts and how each was resolved:

  • DarlingMcpWritePolicy.cs: took dev's AgentActionTools = new HashSet<string>(InvestigationRules.AgentActionToolNames). To keep this PR's tool in the agent arm, set_alert_threshold_override is now the eighth name in InvestigationRules.AgentActionToolNames (Common), the one definition the served set and the brief composer's allowed_actions both read, so it is served and named with no second list. The write-tool list (writes arm) already held every tool from both sides.
  • DarlingMcpInvestigationToolsTests.cs: kept dev's behaviour pin of the served writes under the investigations arm and added set_alert_threshold_override to its literal (seven to eight). Dropped this PR's weaker DoesNotContain line, which the literal supersedes.
  • McpServiceParameterDiSeatCensusTests.cs: served-DI-seat list now holds DarlingAgentActionSeat (Level 2: agents change mute rules and custom alert rules, journaled (#5915) #5934), InvestigationReadPolicy (Practice requests with a score per agent (#5918) #5937) and DarlingMcpServedPolicy (this PR), sorted; the comment carries all three reasons.
  • Darling/README.md: both sections kept (Level 2 section, then Per-server threshold overrides), no duplicated heading.

Pins that moved (the union made set_alert_threshold_override an agent action tool):

  • AgentLevel2ToolPolicyTests (in AgentLevel2MuteRuleToolTests.cs): served agent-arm writes 7 to 8; the instructions census now expects the {writes + admin - 8} ... not served and the 8 mute, custom-rule and threshold override tools are served only to an agent with an agent_action at Level 2 (was 7, "mute and custom-rule"). The instructions text in DarlingMcpInstructions.cs was reworded to name the threshold override tools, since they are now counted in that number. Two tests renamed Seven to Eight.
  • No other pin moved: McpToolsListBudgetTests, DarlingMcpWritePolicyTests, AlertReadFailureSurfaceTests, McpPayloadContractCensusTests, Lite CrossAppMcpToolInventoryPinTests and CrossSkuSurfaceSourceTests pass unchanged. No new census, roster or source-text test added.

Test results (laptop, throwaway PostgreSQL rig stopped afterward):

  • Darling and Lite.Tests build: 0 warnings, 0 errors.
  • Darling, with the rig: *AgentLevel2*, *Level2*, *AgentAction*, *Instructions*, *Investigation*, *ThresholdOverride*, *MuteRule*, every Census, Ratchet and Pin class (including SlowClassCensusTests, no timeout), plus the five named classes: Total 1800, Failed 0, Skipped 0.
  • Lite: CrossAppMcpToolInventoryPinTests, CrossSkuSurfaceSourceTests, every Census, Ratchet and Pin class: Total 345, Failed 0.
  • Not run: the full suite (CI decides). Before the fix to the two pins above, the same selection had those two failures and nothing else.

…ion, set_alert_threshold_override, get_alert_settings server view (#5916)
…in only (#5916 part 4)

Viewer: ThresholdOverridesWindow with ThresholdOverridesViewModel (key and bounds checked before any write), opened from Settings, Alert rules. Web: GET and PUT /api/threshold-overrides and an Admin 'Threshold Overrides' tab; the write is the tool's own body, 403 naming the admin login under the viewer role. The key table moved to PerformanceMonitor.Alerting and the store statements to Darling.Storage so the Viewer, the web route and the tool share them.
@erikdarlingdata
erikdarlingdata marked this pull request as ready for review October 16, 2026 11:13
/// </summary>
internal static AgentActionRefusal? AgentKindRule(DarlingThresholdKeys.Entry entry, int value)
{
var unreachable = value == int.MaxValue

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A Level 2 agent can still turn an alert off per server; the README and this tool's own description say it cannot.

unreachable is value == int.MaxValue or value == 100 on exactly two keys. Every other way to make a bar unreachable passes:

key bounds (DarlingThresholdKeys.cs) value that passes effect on that server
blocking.count_threshold floor…int.MaxValue 2147483646 Blocking Detected can never fire
deadlocks.count_threshold floor…int.MaxValue 2147483646 Deadlocks Detected can never fire
blocking.pg_count_threshold / deadlocks.pg_count_threshold floor…int.MaxValue 2147483646 the two PostgreSQL gates can never fire
pvs.threshold_percent 0…100 100 PVS can never breach
file_growth.volume_percent 0…100 100 File Growth volume arm can never breach
low_disk.threshold_percent + low_disk.threshold_gb 0…100 / 0…max 0 + 0 Low Disk can never fire
long_running_query.threshold_minutes 1…int.MaxValue 2147483646 no query is ever long-running

So the two sentences this PR ships are not true:

  • Darling/README.md (new section): "the agent … cannot set a value that would turn the alert off".
  • the tool Description at line 157: "To silence an alert, mute it: an override moves a threshold and never turns an alert off."

The PR body's "What the coordinator should double-check" is honest that the rule is "deliberately narrow", and which values count as "off" really is a product call — but the shipped docs state the strong version as a guarantee, and an agent reading the tool description is being told a bound that is not enforced. Either tighten the rule (e.g. refuse the key's Max and the degenerate end of each percent/floor key under the agent arm, not just int.MaxValue/100 on two keys) or soften both sentences to say what is actually checked.

ThresholdOverrideTests.TheToolCannotLowerAnAgentsBarToNothing_AndLeavesOrdinaryValuesAlone pins only the two shapes the rule already catches, so nothing here fails on the gap.

Comment on lines +94 to +114
private async void Save_Click(object sender, RoutedEventArgs e)
{
var server = ServerCombo.SelectedItem as ThresholdOverrideServerChoice;
var outcome = await _model.SaveAsync(server?.ServerId, KeyCombo.SelectedItem as string, ValueBox.Text, DatabaseBox.Text);
SetStatus(outcome.Message, !outcome.Ok);
if (outcome.Ok)
{
await LoadAsync();
}
}

private async void Remove_Click(object sender, RoutedEventArgs e)
{
var outcome = await _model.RemoveAsync(OverridesGrid.SelectedItem as ThresholdOverrideViewRow);
SetStatus(outcome.Message, !outcome.Ok);
if (outcome.Ok)
{
await LoadAsync();
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both handlers are unguarded async void: any store failure that is not SQLSTATE 42501 crashes the Viewer.

ViewerDataService.SaveThresholdOverrideAsync / RemoveThresholdOverrideAsync (ViewerDataService.ThresholdOverrides.cs:32, :49) catch exactly one thing:

catch (PostgresException ex) when (ex.SqlState == InsufficientPrivilegeSqlState)

and ThresholdOverridesViewModel.SaveAsync / RemoveAsync catch exactly ViewerReadOnlyException. So a dropped connection, a command timeout, a lock timeout, a serialization failure, or the foreign-key violation a server with no config.config_monitored_servers row would raise all propagate out of these two async void handlers. An exception from async void is posted to the dispatcher, and App.OnDispatcherUnhandledException (App.xaml.cs:188) marks only the Hardcodet tray-tooltip race Handled — everything else is logged and left unhandled, which takes the process down.

LoadAsync just above (line 71) already has the right shape (catch (Exception ex) when (ex is not OperationCanceledException) → SetStatus(..., true)), and NotificationRoutesWindow — the window this one's XAML says it mirrors — wraps every async void handler: AddRoute_Click:103, EditRoute_Click:157, ToggleRoute_Click:175, DeleteRoute_Click:201, each catch (Exception ex) { ShowError(...); }.

This also makes the class doc comment at line 11 ("The store's own refusals … surface as a sentence, never as an exception to the window") and the PR body's "a store refusal becomes a sentence" true only for the 42501 case.

Suggested change
private async void Save_Click(object sender, RoutedEventArgs e)
{
var server = ServerCombo.SelectedItem as ThresholdOverrideServerChoice;
var outcome = await _model.SaveAsync(server?.ServerId, KeyCombo.SelectedItem as string, ValueBox.Text, DatabaseBox.Text);
SetStatus(outcome.Message, !outcome.Ok);
if (outcome.Ok)
{
await LoadAsync();
}
}
private async void Remove_Click(object sender, RoutedEventArgs e)
{
var outcome = await _model.RemoveAsync(OverridesGrid.SelectedItem as ThresholdOverrideViewRow);
SetStatus(outcome.Message, !outcome.Ok);
if (outcome.Ok)
{
await LoadAsync();
}
}
private async void Save_Click(object sender, RoutedEventArgs e)
{
try
{
var server = ServerCombo.SelectedItem as ThresholdOverrideServerChoice;
var outcome = await _model.SaveAsync(server?.ServerId, KeyCombo.SelectedItem as string, ValueBox.Text, DatabaseBox.Text);
SetStatus(outcome.Message, !outcome.Ok);
if (outcome.Ok)
{
await LoadAsync();
}
}
catch (Exception ex) when (ex is not OperationCanceledException)
{
SetStatus("Could not save the override: " + ex.Message, true);
}
}
private async void Remove_Click(object sender, RoutedEventArgs e)
{
try
{
var outcome = await _model.RemoveAsync(OverridesGrid.SelectedItem as ThresholdOverrideViewRow);
SetStatus(outcome.Message, !outcome.Ok);
if (outcome.Ok)
{
await LoadAsync();
}
}
catch (Exception ex) when (ex is not OperationCanceledException)
{
SetStatus("Could not remove the override: " + ex.Message, true);
}
}

};
}

private static int GlobalValue(DarlingAlertReader.AlertSettingsReadRow s, string wire) => wire switch

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trap #3684 — "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted. Alias, do not copy." This is a third copy of the wire → global-value mapping, and it has already drifted.

Three mappings now exist for the same fact:

  1. AlertThresholdKeys.GlobalValue(this IAlertEngineSettings, AlertThresholdKey) (PerformanceMonitor.Alerting/AlertThresholdKey.cs:156) — the one the engine reads through, and whose doc says "The engine reads a tunable member only through this switch, so a census test can hold that it never reads one around IAlertThresholdOverrides.Resolve".
  2. this GlobalValue(AlertSettingsReadRow, string wire) switch.
  3. DarlingThresholdKeys.Entry.Column (wire → config_alert_settings column), which ThresholdOverrideStore.ListAsync uses for the grids' global_value.

The drift is already observable: this switch reads the raw row, while BuildAlertSettingsPayload reports the same two pairs clamped, on purpose —

// DarlingMcpAlertTools.cs:468
warning_per_scheduler = RunnableQueueEvaluator.ClampBars(s.RunnableQueueWarningPerScheduler, s.RunnableQueueCriticalPerScheduler).Warning,

with the comment "Reported through RunnableQueueEvaluator.ClampBars, the way the engine reads them, so the payload never shows a pair the engine would rewrite" (same at :477 for client_timeouts).

server_view now shows exactly such a pair, in the same response. Reachable with nothing but this PR's own tool, since each bar's bounds are checked independently (1 … int.MaxValue) and there is no cross-field rule on the override path, while update_alert_settings does enforce one (DarlingMcpAlertTools.cs:2347, "The cross-field rule (Critical never below Warning) is checked after the loop … and against the stored row inside the write transaction for a lone bar"):

  • global runnable_queue = (warning 8, critical 16).
  • set_alert_threshold_override server_name=srv-7 key=runnable_queue.warning_per_scheduler value=20 — accepted.
  • get_alert_settings server_name=srv-7 → runnable_queue.warning_per_scheduler effective_value 20, runnable_queue.critical_per_scheduler effective_value 16, while the engine evaluates (20, 20) because RunnableQueueEvaluator.ClampBars runs on every evaluation (which IAlertThresholdOverrides.cs:22-24 names as the reason the pairs "need no care here").

So the source/effective_value contract — "the value in effect for this server" — is wrong for those four bars whenever an override inverts the pair. Routing this switch (and Entry.Column) through the one AlertThresholdKeys.GlobalValue definition, and applying the same clamp the payload above applies, removes both the duplicate and the divergence. Nothing currently fails: GetAlertSettings_WithServerName_ShowsTheEffectiveValueAndWhereItComesFrom exercises cpu.threshold_percent and blocking.count_threshold only.

/// after a read that succeeded, and swallows its own write failures.</para>
/// </summary>
/// <summary>
/// Reloads the threshold overrides (#5916). A failed read keeps the overrides already in force and is counted, as the

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: the new method was inserted between LoadMuteRulesAsync's doc block and LoadMuteRulesAsync itself, so the mute-rule prose now documents LoadThresholdOverridesAsync.

The member at line 6774 carries two <summary> elements — the four paragraphs ending "…LoadAsync's expiry purge runs only after a read that succeeded, and swallows its own write failures" (which is about MuteRuleService, config_mute_rules and #3013's surface), then the threshold-override one. LoadMuteRulesAsync at line 6797 is left with no doc comment at all.

Move the new <summary> below the mute-rule block's closing </summary> → above your method, and leave the original block attached to LoadMuteRulesAsync.

case "refused":
case "invalid":
return (400, new JsonObject
{

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: precondition has two causes and this collapses them to the wrong sentence for one of them.

SetAsync answers precondition when HoldsGrantAsync is false or when the store raises 42501. On a bring-your-own store that has migrated to V191 but has not re-run provision-roles.sql step 3j, the web login genuinely is the admin role and still has no grant on the single new table — and this route tells the operator the opposite: "needs a store login with the admin role … set postgres.webConnectionString to an admin login."

The README section this PR adds promises the other message for exactly that case: "Re-run Darling/tools/provision-roles.sql once after upgrading; until then the tool answers precondition and names the script." ListBodyAsync only reaches DarlingMcpThresholdOverrideTools.PreconditionMessage when the SELECT also 42501s, so an admin login holding SELECT but not INSERT/UPDATE/DELETE gets a clean list, can_edit: false, note: null, and the Admin page's OVERRIDES_NOTE_READ_ONLY strip — three surfaces all naming the wrong remedy.

Carrying answer["message"] through on the precondition arm (or appending PreconditionMessage to RefusedMessage) would name the script where the README says it will.

@claude

claude Bot commented Oct 16, 2026

Copy link
Copy Markdown

Smaller notes (none of these are blockers)

1. "a fifth tab" is the sixth. The body's ## Part 4 says "admin.js gains a fifth tab, 'Threshold Overrides'" and ## UI smoke repeats "now draws a fifth tab". TABS at admin.js:660 has six entries (Servers, Notification Routes, Alert Settings, Report Feedback, Pending Notes, Threshold Overrides), and the merge note at the bottom of the body already corrects itself ("the header comment now says 'Six tabs'"). Worth fixing before the CHANGELOG entry is lifted out, and worth knowing for the #5929 baseline: the admin-servers page gains two tab links against the pre-#5926 baseline, not one.

2. ThresholdOverrideStore.ListSql uses bare table aliases. FROM config.config_threshold_overrides o JOIN config.config_monitored_servers s ON s.server_id = o.server_id — CONTRIBUTING's style section asks for AS on table aliases. It is the T-SQL section and plenty of Darling's PostgreSQL reads already do this (DarlingAdminServersReader.AdminServersSql:74-75), so a nit, flagged only because the convention is written down.

3. A server removed from config but not yet mirrored can FK-fail instead of refusing. DarlingServerResolver.LoadEnabledServersSql and DarlingWebThresholdOverrides.ListBodyAsync's server list both read collect.servers WHERE is_enabled, while config_threshold_overrides.server_id has its foreign key on config.config_monitored_servers (V191). Between a remove_server / web / Viewer removal and the next reload's deleted-server mirror (DarlingObservability, the #2030 half of SyncEnabledStatesSql), the server is still enabled in collect.servers and absent from config, so the upsert raises 23503 — which reaches an operator as McpHelpers.FormatError's raw text on MCP and as a flat 500 "The override could not be saved." on the web, rather than a sentence saying the server is gone. Narrow and transient; mentioned because the two reads and the write disagree about which table is the registry.

4. DarlingThresholdOverrides.Replace skips silently where LoadAsync logs. Accept returns the problem message in both paths, but Replace (DarlingThresholdOverrides.cs:1100-1111) throws it away while LoadAsync logs a warning naming the server, key and database. The doc on Replace says it is for "the loader's own tests, and any caller that read the table" — if a non-test caller ever appears, a skipped row goes unrecorded there.

5. previous_override truncates. applied.Previous is { } p ? (int?)(int)p : null casts the stored double precision to int, so a row holding 0.5 (which the column allows, and which the loader deliberately refuses to apply) is reported back as 0 rather than as the value being replaced. Every writer in the tree writes integers, so this is only reachable through a hand-edited row.

claude[bot]
claude Bot previously requested changes Oct 16, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: CHANGES REQUESTED

Two blockers inline: the Level 2 kind rule does not enforce what the README and the tool description promise, and the Viewer window's two async void handlers can end the process. A third finding (a duplicated global-value mapping that has already drifted) plus five smaller notes are posted separately.

Claims

  • VERIFIED — loader implements IAlertThresholdOverrides from the table, one in-memory snapshot, no sweep-path I/O, loads at start and each control-plane reload, a failed load keeps the snapshot — DarlingThresholdOverrides.cs:54-97; DarlingWorker.cs:3376, :6703, catch at :6786
  • VERIFIED — a row that is not tunable / not an integer / out of bounds / database on a server-only key is skipped and logged, never applied — Accept:1113-1122; ThresholdOverrideTests.ARowTheGlobalSettingWouldRefuse_IsNotApplied
  • VERIFIED — a database override wins for that database, a server override covers the server's other databases — ResolveWire:1140-1142; ThresholdOverrideTests.AnOverride_MovesItsServer_AndLeavesTheOther_AndADatabaseOverrideAppliesToThatDatabaseOnly
  • VERIFIED — DarlingThresholdKeys is every #5910 key plus the two PostgreSQL counts, wire-named, with update_alert_settings' per-field bounds — 23 entries at DarlingThresholdKeys.cs:35-57, pinned as Enum.GetValues<AlertThresholdKey>().Length + 2; all 23 Column names exist on config_alert_settings
  • VERIFIED — BuildAlertEngine passes the loader (DarlingWorker.cs:7660) and the two PostgreSQL count checks resolve through ResolveWire with snapshot.ServerKey (:8517, :8727), which is runtime.ServerId.ToString(InvariantCulture) (:7739) — the same identity as config_threshold_overrides.server_id
  • VERIFIED — Lite passes nothing and is untouched — no Lite/ file in the diff; IAlertThresholdOverrides.cs:17-18
  • VERIFIED — in WriteTools and AgentActionTools so both arms offer it (DarlingMcpWritePolicy.cs:79, :118-121, Offers:168-177); writes arm ignores agent_action, investigations arm requires it, gates to the request's server (InvestigationRequestStore.AgentAction.cs:290-292) and records agent:<id> + the request id (journal :182/:204)
  • VERIFIED — the four refusal classes and precondition naming provision-roles.sql — DarlingThresholdKeys.cs:2900-2929, HoldsGrantAsync:213
  • VERIFIED — get_alert_settings gains an optional server_name and server_view, and the web /api/read passes it — DarlingMcpAlertTools.cs:305, :330-342; DarlingWebEndpoints.cs:6644; both existing callers still compile
  • VERIFIED — the web half: 200/400/415/403-with-RefusedMessage (ChangeAsync:1390-1405), every body through DarlingWebStatementSweep.JsonText, read-only seats refused by DarlingWebSeat.IsRequestAllowed:110-123, the form gated on both can_edit flags, ThresholdOverrideStore shared by tool/route/Viewer, DarlingThresholdKeys public in Alerting (...Viewer.csproj:52), both routes added to ReadAdmissionCoverageCensusTests, no new census
  • UNVERIFIED — every rig and run count (22/25, 11, 18, 11, 3; "3300 run, 0 failed"; "2888 run"; "1140 Failed: 0") and "Build 0 warnings" — needs dotnet build and a store with DARLING_TEST_PG
  • UNVERIFIED — "McpToolsListBudgetTests passes", "default 163 to 164, catalog 188 to 189" — the catalog figure is machine-pinned at DarlingMcpWritePolicyTests.cs:242 but needs a build; the default-served figure is nowhere in the tree
  • UNVERIFIED — "node --check passes" on admin.js — not executed here
  • UNVERIFIED — "the window was built but not opened by hand" — needs a desktop session, which is where blocker 2 surfaces
  • REFUTED — "admin.js gains a fifth tab" (## Part 4, repeated in ## UI smoke) — TABS at admin.js:660 has six entries; the body's own merge note says "Six tabs"
  • REFUTED — "a store refusal becomes a sentence" / ThresholdOverridesWindow.xaml.cs:11 "never as an exception to the window" — only for SQLSTATE 42501: ViewerDataService.ThresholdOverrides.cs:32/:49 catch that one state, the view-model only ViewerReadOnlyException, and Save_Click/Remove_Click are bare async void, so anything else reaches App.OnDispatcherUnhandledException:188, which marks only the tray-tooltip race Handled
  • REFUTED — the README's "the agent … cannot set a value that would turn the alert off" and the description's "never turns an alert off" — AgentKindRule:211 refuses int.MaxValue and 100 on two keys only; blocking.count_threshold = 2147483646, pvs.threshold_percent = 100, file_growth.volume_percent = 100, low_disk.threshold_percent = 0 all pass and make their alert unreachable

Riskiest lines

  1. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpThresholdOverrideTools.cs:211 — the only thing between a Level 2 agent and a per-server alert switch-off, enumerating two shapes out of many — pinned by ThresholdOverrideTests.TheToolCannotLowerAnAgentsBarToNothing_AndLeavesOrdinaryValuesAlone for the two it catches; the bypasses UNPINNED
  2. Darling/PerformanceMonitor.Darling.Viewer/ThresholdOverridesWindow.xaml.cs:94 and :105 — unguarded async void — UNPINNED (ThresholdOverridesViewModelTests drives the view-model, never the window)
  3. Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs:8517 and :8727 — the two PostgreSQL bars now come from the snapshot on the sweep path — resolution pinned by ThresholdOverrideTests.EachPostgreSqlCountThreshold_FollowsItsServersOverride, the loader by ThresholdOverrideLiveTests.WritesArm_SetsUpdatesAndClears_IgnoresAgentAction_AndTheLoaderReadsIt, the call sites UNPINNED

Traps near this diff

  • "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — applies — GlobalValue:308 is a second wire→global-value switch beside AlertThresholdKeys.GlobalValue (AlertThresholdKey.cs:156), Entry.Column a third; already drifted, since this one reads the raw row while DarlingMcpAlertTools.cs:468/:477 report the same bars clamped "so the payload never shows a pair the engine would rewrite"
  • "Lite and Darling ship in parity: a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply — the seam is optional, not defaulted (IAlertThresholdOverrides.cs:17-18), and the README's section ends "Lite keeps global thresholds only and has no override path"
  • "A default interface member compiles a parity gap away (#5640)" — does not apply — both new interfaces declare bodiless members (ThresholdOverridesViewModel.cs:2392-2396, IAlertThresholdOverrides.cs:36)
  • "hours_back, limit, top, days_back are refused when out of range, never clamped" — does not apply — ValidateValue:2921-2925 refuses and SetAsync returns before any statement (:146-147), pinned by ThresholdOverrideLiveTests.ARefusedKeyOrValue_LeavesTheTableEmpty
  • "Truncation inferred from a full page" / "A total_* assigned from a .Count (#3699)" — do not apply — neither new read is paged: ThresholdOverrideStore.ListSql and BuildServerViewAsync carry no LIMIT, and no payload reports a total or truncated
  • "A JSON number above 2^53 is rounded by every JavaScript client (#3487)" — does not apply — the only id on the wire is request_id, a uuid string (DarlingWebThresholdOverrides.cs:1316); server_id is int32
  • "Ruling (#5833): tests are load-bearing … never answer a trap with a test that reads source text" — does not apply — the new classes are behaviour tests (recording fake gateway, live store, Node harness over the shipped admin.js); both census touches are entries in existing rosters

Divergence (blind read vs PR body)

All three in the same direction: the body and the shipped docs state a guarantee one notch stronger than the code enforces.

  1. Kind rule reach. The body is candid ("deliberately narrow … a product call"), which is why this is divergence and not misstatement — but the README and the tool Description state the strong version, and an agent reads the description.
  2. "a store refusal becomes a sentence." True of the view-model, not of the window; the failure mode is not a bad sentence but the Viewer exiting.
  3. One definition, implemented twice. "The one key table" holds for the write path — one registry, one store, three surfaces, which is the good shape here. The read path did not follow: server_view's global value is a fresh switch that reads raw where the enclosing payload reads clamped, so one response can carry two numbers for one global threshold.

"No behaviour change to the tool" holds for the store extraction; Entry.Column and the grids' global_value are new behaviour beside an existing tool, described elsewhere in the body, so minor.

Reviewer checklist

  • Open the Viewer's Threshold Overrides window against a store you can break mid-save (kill the backend, or force a 1 s command timeout) and see whether Save and Remove show a sentence or end the process
  • Run the three new live classes with DARLING_TEST_PG set, confirm the 3 + 11 counts, then dotnet build for "0 warnings"
  • Confirm DarlingMcpWritePolicyTests agrees with "189 tools / 94 Darling-only" and McpToolsListBudgetTests still passes
  • On a BYO store at V191 without provision-roles.sql re-run, compare the sentence the tool, the list note and the Admin tab each give
  • As a Level 2 agent, try key=blocking.count_threshold value=2147483646 and decide whether that outcome is the product intent

@erikdarlingdata
erikdarlingdata dismissed claude[bot]’s stale review October 16, 2026 12:18

Claude posting for Erik Darling. Both blockers, the duplicated mapping, the five notes and the CI reds are answered in the body's Review round 1 section (head 99f61f1); the next round reviews that head.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant