diff --git a/Darling/Darling.Tests/CollectorStallProbeStoreTests.cs b/Darling/Darling.Tests/CollectorStallProbeStoreTests.cs index bd9b86c6c9..b7dab3f540 100644 --- a/Darling/Darling.Tests/CollectorStallProbeStoreTests.cs +++ b/Darling/Darling.Tests/CollectorStallProbeStoreTests.cs @@ -40,7 +40,7 @@ public class CollectorStallProbeStoreTests private const string TableName = "collector_stall_probes"; [Fact] - public void TheRungIsRegisteredAtTheTopOfADenseLadder() + public void TheRungIsRegisteredInADenseLadder() { var versions = PgMigrations.Scripts.Select(s => s.Version).ToList(); @@ -49,7 +49,13 @@ public void TheRungIsRegisteredAtTheTopOfADenseLadder() Assert.Equal(StorageVersion.SchemaVersion, PgMigrations.Scripts[^1].Version); Assert.Equal(StorageVersion.SchemaVersion, versions.Max()); - Assert.Equal(RungVersion, StorageVersion.SchemaVersion); + /* NOT "this rung is the top" any more: V113 (#2138 phase 1) is, and it carries that guard in its + own suite. A rung that was top when its test was written cannot keep asserting it — the claim + belongs to whichever rung actually is, or every new rung breaks every older rung's suite. What + stays here is that this rung is IN the ladder and no higher than its head. */ + Assert.True( + RungVersion <= StorageVersion.SchemaVersion, + $"V{RungVersion} sits above StorageVersion.SchemaVersion ({StorageVersion.SchemaVersion}), so it can never apply"); Assert.Equal(versions.Distinct().OrderBy(v => v), versions); var above = versions.Where(v => v > 45).OrderBy(v => v).ToList(); diff --git a/Darling/Darling.Tests/CollectorStallProbeViewerGateTests.cs b/Darling/Darling.Tests/CollectorStallProbeViewerGateTests.cs index 1406216f4b..a7cab75041 100644 --- a/Darling/Darling.Tests/CollectorStallProbeViewerGateTests.cs +++ b/Darling/Darling.Tests/CollectorStallProbeViewerGateTests.cs @@ -31,13 +31,20 @@ public class CollectorStallProbeViewerGateTests /// The version a store one rung behind this one reports. private const int PreviousVersion = 111; + /// The rung this suite is about. Read from the store-side suite so the two cannot disagree. + private const int RungVersion = CollectorStallProbeStoreTests.RungVersion; + /// The table the rung creates. private const string TableName = "collector_stall_probes"; /// - /// The connect-time gate. A TABLE sentinel, because the table is the only object the rung creates. Being - /// the TOP rung, a fully-migrated store must map to exactly this version or the viewer refuses a store - /// that is perfectly current — permanently, because no later upgrade changes the answer. + /// The connect-time gate. A TABLE sentinel, because the table is the only object the rung creates. + /// + /// This rung is no longer the top one — V113 (#2138 phase 1) is — so the "a fully-migrated store + /// maps to exactly THIS version" clause has moved to that rung's suite, where it is true. Two things + /// here had to stop assuming it: the sentinel's ordinal is no longer the last one, and the + /// one-rung-behind check has to switch off every LATER sentinel too, or it measures the newest rung + /// instead of this one. /// [Fact] public void TheProbeAsksForTheTable_AndMapsAFullyMigratedStoreToThisRung() @@ -56,22 +63,22 @@ public void TheProbeAsksForTheTable_AndMapsAFullyMigratedStoreToThisRung() .GetMethod("MapProbedSchemaVersion", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static)!; var arity = method.GetParameters().Length; - /* The sentinel count and the ordinal have to agree, or the ordinal literal above is pinning a - position that no longer exists. */ - Assert.Equal(arity - 1, ProbeOrdinal); + /* The ordinal has to be a position that exists. It was arity - 1 while this was the top rung; a + later rung appends a sentinel and that equality would fail for every rung but the newest, which + is a pin about the ladder's length rather than about this rung. */ + Assert.True( + ProbeOrdinal < arity, + $"sentinel ordinal {ProbeOrdinal} is outside the probe's {arity} parameters"); - /* Every sentinel true = a fully-migrated store, which must map to THIS rung. As the top rung this is - also the "and no more than that" guard: a later rung appending a sentinel without its own arm - would leave this returning 112 for a store that is actually further along. Built by reflection so - the arity tracks the signature — the literal-true form silently defaults a newly added sentinel to - false and maps one version low. */ - var all = Enumerable.Repeat((object)true, arity).ToArray(); - Assert.Equal(StorageVersion.SchemaVersion, (int)method.Invoke(null, all)!); + /* A store migrated to exactly THIS rung: every sentinel up to and including this one true, every + later one false. Built by reflection so the arity tracks the signature — the literal-true form + silently defaults a newly added sentinel to false and maps one version low. */ + var throughMine = Enumerable.Range(0, arity).Select(i => (object)(i <= ProbeOrdinal)).ToArray(); + Assert.Equal(RungVersion, (int)method.Invoke(null, throughMine)!); - /* One rung behind: every sentinel present EXCEPT this one must report 111, not 112. Without this the + /* One rung behind: this rung's sentinel absent as well must report 111, not 112. Without it the arm above could be satisfied by an unconditional return and nothing would notice. */ - var allButMine = Enumerable.Repeat((object)true, arity).ToArray(); - allButMine[ProbeOrdinal] = false; + var allButMine = Enumerable.Range(0, arity).Select(i => (object)(i < ProbeOrdinal)).ToArray(); Assert.Equal(PreviousVersion, (int)method.Invoke(null, allButMine)!); } diff --git a/Darling/Darling.Tests/DarlingManagedRolesTests.cs b/Darling/Darling.Tests/DarlingManagedRolesTests.cs index dcd45d13d4..0e79f9e9b8 100644 --- a/Darling/Darling.Tests/DarlingManagedRolesTests.cs +++ b/Darling/Darling.Tests/DarlingManagedRolesTests.cs @@ -150,7 +150,10 @@ public void ViewerRestrictedConfigTables_SecretAndNonSecretColumns_AreDisjointAn { var expectedSecrets = new Dictionary(StringComparer.Ordinal) { - ["config_monitored_servers"] = new[] { "encrypted_password" }, + /* V113 (#2138 phase 1): the remediation credential's blob is the same kind of thing as the + monitoring one beside it — a DPAPI secret — and it authenticates a WRITE, so if anything on + this table is secret it is. */ + ["config_monitored_servers"] = new[] { "encrypted_password", "remediation_encrypted_password" }, ["config_command"] = new[] { "args_json" }, /* generic_url is a bearer secret like the sibling webhook URLs, and generic_headers holds the Authorization token itself (#1506 / V26). pagerduty_routing_key is the Events API v2 integration diff --git a/Darling/Darling.Tests/DarlingRetentionTests.cs b/Darling/Darling.Tests/DarlingRetentionTests.cs index 09d68ad327..5d9120bcce 100644 --- a/Darling/Darling.Tests/DarlingRetentionTests.cs +++ b/Darling/Darling.Tests/DarlingRetentionTests.cs @@ -471,9 +471,13 @@ horizon in the store (90-day alert history, 60-day collection_log, 30-day base). foreach (var (ageDays, decision) in new[] { (400, "would_force"), (100, "blocked") }) { using var insert = new NpgsqlCommand( + /* actor is named because V113 (#2138 phase 1) dropped its DEFAULT: it is the column the + bot's own-forces-only invariant reads, and an INSERT that omits it must fail rather + than silently claim to be the bot. This raw INSERT is exactly the shape that would + have — it did, with 23502, which is the guard working. */ "INSERT INTO collect.plan_force_actions" - + " (action_time, server_id, server_name, database_name, query_id, plan_id, action, mode, decision, outcome)" - + " VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10)", connection); + + " (action_time, server_id, server_name, database_name, query_id, plan_id, action, mode, actor, decision, outcome)" + + " VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11)", connection); insert.Parameters.AddWithValue(utcNow.AddDays(-ageDays)); insert.Parameters.AddWithValue(TestServerId); insert.Parameters.AddWithValue("retention-e2e"); @@ -482,6 +486,7 @@ horizon in the store (90-day alert history, 60-day collection_log, 30-day base). insert.Parameters.AddWithValue(2L); insert.Parameters.AddWithValue("force"); insert.Parameters.AddWithValue("dry_run"); + insert.Parameters.AddWithValue("bot"); insert.Parameters.AddWithValue(decision); insert.Parameters.AddWithValue("journaled"); await insert.ExecuteNonQueryAsync(ct); diff --git a/Darling/Darling.Tests/MigrationDataMovingRungCensusPins.cs b/Darling/Darling.Tests/MigrationDataMovingRungCensusPins.cs index 6fda17405b..b180b48b83 100644 --- a/Darling/Darling.Tests/MigrationDataMovingRungCensusPins.cs +++ b/Darling/Darling.Tests/MigrationDataMovingRungCensusPins.cs @@ -110,6 +110,19 @@ public sealed class MigrationDataMovingRungCensusPins SetsTheFloor: true, "CREATE INDEX over the populated collect.pg_deadlocks hypertable - index-only rung, so a " + "store that sat on V103 for a release pays the whole build here"), + new( + 113, + SetsTheFloor: false, + "CREATE INDEX on collect.plan_force_actions (created V107), for the actor-filtered " + + "pending-review read. Real DML shape on a pre-existing table, but the table's SIZE is " + + "bounded by construction and the bound is small: one row per force/unforce decision per " + + "server, capped by the bot's per-query cooldown and per-server daily budget (3), and purged " + + "at 365 days - so its ceiling across a 42-server fleet is ~46k rows, and it is EMPTY on " + + "every store today because the bot ships off. An index build at that scale is milliseconds, " + + "which is the opposite of V104's case: that one is a hypertable carrying a real collected " + + "series. The two ADD COLUMNs in the same rung are not findings and that was measured rather " + + "than assumed - the volatile-DEFAULT shape needs DEFAULT ( and this rung's default is " + + "the literal 'bot', and ALTER COLUMN ... DROP DEFAULT is catalog-only"), ]; /// diff --git a/Darling/Darling.Tests/OperatorRemediationFlowTests.cs b/Darling/Darling.Tests/OperatorRemediationFlowTests.cs new file mode 100644 index 0000000000..cab0a0073a --- /dev/null +++ b/Darling/Darling.Tests/OperatorRemediationFlowTests.cs @@ -0,0 +1,454 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Linq; +using PerformanceMonitor.Analysis; +using Xunit; + +namespace Darling.Tests; + +/// +/// The #2138 phase-1 evict-then-observe state machine, case by case. The contracts: +/// +/// the two window limbs are NOT interchangeable — plan identity needs one compile, cost needs the +/// executions floor, and a window the timeout closed cannot support a cost verdict; +/// the force is offered for exactly ONE verdict, and offering it is a property of the returned +/// value so no caller can re-derive it differently; +/// the floors come from and are not restated here — pinned by +/// driving the boundary off the settings value rather than off a literal. +/// +/// +public sealed class OperatorRemediationFlowTests +{ + private const string RegressedHash = "0x2222222222222222"; + + private const string OtherHash = "0x3333333333333333"; + + private const double Baseline = 50000; + + private static readonly int Floor = ForcePlanBotSettings.Default.MinReviewExecutions; + + private static readonly TimeSpan Window = + TimeSpan.FromMinutes(ForcePlanBotSettings.Default.ObservationWindowMinutes); + + private static RemediationObservationResult Observe( + long executions, + TimeSpan elapsed, + string? activePlanHash, + double? observedCpu) => + OperatorRemediationFlow.Observe( + new RemediationObservation(executions, elapsed, activePlanHash, observedCpu), + RegressedHash, + Baseline, + Floor, + Window); + + /* ---------------- the window ---------------- */ + + [Fact] + public void BelowBothLimbs_TheWindowIsStillOpen_AndNothingIsOffered() + { + var result = Observe(Floor - 1, Window - TimeSpan.FromMinutes(1), OtherHash, 1000); + + Assert.Equal(RemediationObservationVerdict.StillObserving, result.Verdict); + Assert.Equal(ObservationWindowLimb.Open, result.Limb); + Assert.False(result.ForceOffered); + } + + /// + /// The executions limb fires AT the floor and not one execution before it, and the boundary is read + /// off rather than a literal 25 — so a flow + /// that restated the number, or ignored the parameter, fails here rather than agreeing by coincidence. + /// + [Fact] + public void TheExecutionsLimbFiresAtTheSettingsFloor_NotAtALiteral() + { + var justBelow = new RemediationObservation(Floor - 1, TimeSpan.Zero, OtherHash, 1000); + var atTheFloor = new RemediationObservation(Floor, TimeSpan.Zero, OtherHash, 1000); + + Assert.Equal( + ObservationWindowLimb.Open, + OperatorRemediationFlow.Limb(justBelow, Floor, Window)); + Assert.Equal( + ObservationWindowLimb.Executions, + OperatorRemediationFlow.Limb(atTheFloor, Floor, Window)); + + /* And it moves WITH the setting. A hardcoded 25 would keep the two assertions above passing + while failing this one, which is the whole point of the parameter. */ + var raised = Floor * 4; + Assert.Equal( + ObservationWindowLimb.Open, + OperatorRemediationFlow.Limb(new RemediationObservation(Floor, TimeSpan.Zero, OtherHash, 1000), raised, Window)); + } + + [Fact] + public void TheElapsedLimbIsATimeout_NotASecondMeasurement() + { + var result = Observe(3, Window, OtherHash, 1000); + + Assert.Equal(ObservationWindowLimb.Elapsed, result.Limb); + + /* Three executions of a different plan, at 2% of baseline. Cheap-looking, and refused: the window + closed on the clock, so there is no cost evidence to be had. Calling this recovery is exactly + what the two-limb split exists to prevent. */ + Assert.Equal(RemediationObservationVerdict.Inconclusive, result.Verdict); + Assert.False(result.ForceOffered); + } + + /// + /// When both limbs are satisfied the EXECUTIONS limb is reported. It has to win: the elapsed limb + /// would suppress a cost verdict the executions actually support, so reporting the timeout for a + /// window that also gathered enough evidence would throw away a real measurement. + /// + [Fact] + public void WhenBothLimbsAreSatisfied_TheOneCarryingEvidenceWins() + { + var result = Observe(Floor * 2, Window * 2, OtherHash, Baseline * 0.2); + + Assert.Equal(ObservationWindowLimb.Executions, result.Limb); + Assert.Equal(RemediationObservationVerdict.OptimizerRecovered, result.Verdict); + } + + /// + /// An open window is not decided, even when the hash already matches. This is the case review + /// asked about (#3170) and it was genuinely uncovered — the wait fell out of putting the window guard + /// first, not out of a decision — so it is pinned here with its reason rather than left to read as an + /// accident either way. + /// + /// The reason the wait is right is the INSTRUMENT, not caution about transient recompiles. A + /// targeted DBCC FREEPROCCACHE(plan_handle) evicts the plan CACHE and Query Store keeps its plan + /// row, so the regressed hash is present the instant after the eviction and stays present. + /// is therefore only meaningful once post-eviction + /// executions have been attributed to a plan — before that, an identity arm would not be catching an + /// early recompile, it would be reading the pre-eviction plan and offering a force on it, on the first + /// call, every time. + /// + /// Cheap to get wrong in the direction that acts: firing early would make the force the quick + /// answer and — which needs the + /// executions floor — the slow one, on a lever whose premise is that the cheapest fix pins nothing. + /// + [Fact] + public void AMatchingHashWhileTheWindowIsStillOpen_IsNotYetAVerdict() + { + /* One execution, seconds after the eviction, already reporting the regressed hash — exactly the + shape review described as "the evidence arrived on compile #1". */ + var result = Observe(1, TimeSpan.FromSeconds(5), RegressedHash, Baseline); + + Assert.Equal(RemediationObservationVerdict.StillObserving, result.Verdict); + Assert.Equal(ObservationWindowLimb.Open, result.Limb); + Assert.False( + result.ForceOffered, + "a force must not be offered on a plan hash read before any post-eviction execution was " + + "attributed — after a plan-cache eviction that hash is the pre-eviction plan's"); + } + + /// + /// The discriminating control for the pin above, and the reason it is not just "everything returns + /// StillObserving while the window is open". The SAME matching hash, once a limb closes, does reach + /// and does offer the force — so the + /// pin above is about the WINDOW and not about the hash comparison being broken. + /// + [Fact] + public void TheSameMatchingHash_BecomesAVerdictOnceEitherLimbCloses() + { + var byExecutions = Observe(Floor, TimeSpan.FromSeconds(5), RegressedHash, Baseline); + Assert.Equal(ObservationWindowLimb.Executions, byExecutions.Limb); + Assert.Equal(RemediationObservationVerdict.RegressedPlanReturned, byExecutions.Verdict); + Assert.True(byExecutions.ForceOffered); + + var byTimeout = Observe(1, Window, RegressedHash, Baseline); + Assert.Equal(ObservationWindowLimb.Elapsed, byTimeout.Limb); + Assert.Equal(RemediationObservationVerdict.RegressedPlanReturned, byTimeout.Verdict); + Assert.True(byTimeout.ForceOffered); + } + + /// + /// No verdict of any kind escapes an open window — asserted over every observation shape that reaches + /// a verdict once a limb closes, rather than for the matching-hash case alone. Exhaustive because the + /// failure mode is an arm someone later hoists above the window guard for one verdict and not the + /// others, which is precisely what the review question proposed. + /// + [Fact] + public void NoVerdictEscapesAnOpenWindow() + { + var shapes = new[] + { + ("regressed plan back", RegressedHash, (double?)Baseline), + ("cheaper plan", OtherHash, Baseline * 0.2), + ("worse plan", OtherHash, Baseline * 2), + ("indistinguishable plan", OtherHash, Baseline), + ("no cost yet", OtherHash, null), + }; + + var checkedShapes = 0; + foreach (var (label, hash, cpu) in shapes) + { + /* Below both limbs: one execution, five seconds. */ + var open = Observe(1, TimeSpan.FromSeconds(5), hash, cpu); + Assert.Equal(ObservationWindowLimb.Open, open.Limb); + Assert.Equal(RemediationObservationVerdict.StillObserving, open.Verdict); + Assert.False(open.ForceOffered, $"{label} offered a force on an open window"); + + /* Positive control per shape: the same inputs DO reach a real verdict once the window closes, + so the assertion above is about the window rather than about an input that never decides + anything. Without this the loop would pass for a shape that is inconclusive regardless. */ + var closed = Observe(Floor, Window, hash, cpu); + Assert.NotEqual(RemediationObservationVerdict.StillObserving, closed.Verdict); + checkedShapes++; + } + + Assert.Equal(shapes.Length, checkedShapes); + } + + /* ---------------- the four verdicts ---------------- */ + + [Fact] + public void ACheaperDifferentPlan_IsOptimizerRecovered_AndOffersNoForce() + { + var result = Observe(Floor, TimeSpan.Zero, OtherHash, Baseline * 0.2); + + Assert.Equal(RemediationObservationVerdict.OptimizerRecovered, result.Verdict); + Assert.False(result.ForceOffered); + Assert.Equal( + OperatorRemediationFlow.DecisionOptimizerRecovered, + OperatorRemediationFlow.DecisionFor(result.Verdict)); + } + + [Fact] + public void TheRegressedPlanComingBack_OffersTheForce_AsTheSecondDecision() + { + var result = Observe(Floor, TimeSpan.Zero, RegressedHash, Baseline); + + Assert.Equal(RemediationObservationVerdict.RegressedPlanReturned, result.Verdict); + Assert.True(result.ForceOffered); + } + + /// + /// Plan IDENTITY does not need the cost floor: one compile settles which plan the optimizer chose, so + /// the regressed plan returning is a legitimate verdict on a window the timeout closed with three + /// executions. This is the arm that would be lost by giving both limbs the same evidence requirement, + /// and it is the arm the whole evict-first strategy turns on — "the eviction changed nothing" is the + /// answer that justifies the force. + /// + [Fact] + public void TheRegressedPlanComingBack_IsJudgedOnIdentity_EvenOnTheTimeoutLimb() + { + var result = Observe(3, Window, RegressedHash, null); + + Assert.Equal(ObservationWindowLimb.Elapsed, result.Limb); + Assert.Equal(RemediationObservationVerdict.RegressedPlanReturned, result.Verdict); + Assert.True(result.ForceOffered); + } + + /// + /// Query Store renders a plan hash as 0x-prefixed hex, and the two sides of this comparison come from + /// different places — the persisted target, and a live read — so neither case nor prefix is guaranteed + /// to agree even when the hashes do. A comparison that missed on either would report the regressed + /// plan as "some other plan, indistinguishable in cost" and silently drop the force. + /// + [Theory] + [InlineData("0x2222222222222222")] + [InlineData("2222222222222222")] + [InlineData("0X2222222222222222")] + [InlineData(" 0x2222222222222222 ")] + public void PlanHashComparisonSurvivesPrefixCaseAndPadding(string activeHash) + { + Assert.Equal( + RemediationObservationVerdict.RegressedPlanReturned, + Observe(Floor, TimeSpan.Zero, activeHash, Baseline).Verdict); + } + + [Fact] + public void AMeasurablyWorsePlan_JournalsAndStops_WithNoForceOffered() + { + var result = Observe(Floor, TimeSpan.Zero, OtherHash, Baseline * 2); + + Assert.Equal(RemediationObservationVerdict.Worse, result.Verdict); + Assert.False(result.ForceOffered); + Assert.Equal( + OperatorRemediationFlow.DecisionWorseAfterEvict, + OperatorRemediationFlow.DecisionFor(result.Verdict)); + } + + /// + /// Inside the dead band a different plan is neither recovery nor worse. Without the band, cpu/exec + /// drifting a few percent for reasons unrelated to which plan compiled would be reported as a verdict. + /// + [Theory] + [InlineData(0.9)] + [InlineData(1.0)] + [InlineData(1.1)] + public void ADifferentPlanIndistinguishableInCost_IsInconclusive(double ratio) + { + var result = Observe(Floor, TimeSpan.Zero, OtherHash, Baseline * ratio); + + Assert.Equal(RemediationObservationVerdict.Inconclusive, result.Verdict); + Assert.False(result.ForceOffered); + } + + /// + /// The band's edges are inclusive on the verdict side: exactly at the bar counts. Pinned because + /// "at least 25% better" and "more than 25% better" are one character apart and the difference decides + /// whether a borderline eviction reports recovery. + /// + [Fact] + public void ExactlyAtTheBar_Counts() + { + Assert.Equal( + RemediationObservationVerdict.OptimizerRecovered, + Observe(Floor, TimeSpan.Zero, OtherHash, Baseline * OperatorRemediationFlow.MaterialChangeRatio).Verdict); + + Assert.Equal( + RemediationObservationVerdict.Worse, + Observe(Floor, TimeSpan.Zero, OtherHash, Baseline / OperatorRemediationFlow.MaterialChangeRatio).Verdict); + } + + /* ---------------- honest absences ---------------- */ + + /// + /// A null active plan hash means the server has not attributed a post-evict compile — the OPPOSITE of + /// evidence that the regressed plan is back. Pinned because a hash comparison that treats two absences + /// as equal would report every un-recompiled query as the regressed plan returning, and offer a force + /// on it. + /// + [Fact] + public void ANullActivePlanHash_NeverReadsAsTheRegressedPlanReturning() + { + var result = Observe(Floor, TimeSpan.Zero, null, Baseline); + + Assert.NotEqual(RemediationObservationVerdict.RegressedPlanReturned, result.Verdict); + Assert.False(result.ForceOffered); + } + + [Fact] + public void AnAbsentRegressedHashOnTheTargetSide_AlsoNeverMatches() + { + var result = OperatorRemediationFlow.Observe( + new RemediationObservation(Floor, TimeSpan.Zero, null, Baseline), + regressedPlanHash: null, + Baseline, + Floor, + Window); + + Assert.NotEqual(RemediationObservationVerdict.RegressedPlanReturned, result.Verdict); + } + + /// + /// A cost the server could not give us is not a cost of zero. Written as a loop over the three + /// unusable shapes rather than as [InlineData(null)], which binds the null to the attribute's + /// whole params array instead of to the parameter — a real ambiguity, not a formatting choice. + /// + [Fact] + public void AnUnusableObservedCost_IsInconclusive_RatherThanAVerdict() + { + foreach (var observedCpu in new double?[] { null, double.NaN, double.PositiveInfinity }) + { + Assert.Equal( + RemediationObservationVerdict.Inconclusive, + Observe(Floor, TimeSpan.Zero, OtherHash, observedCpu).Verdict); + } + } + + [Fact] + public void AnUnusableBaseline_IsInconclusive_RatherThanAVerdict() + { + foreach (var baseline in new[] { 0d, -1d, double.NaN }) + { + var result = OperatorRemediationFlow.Observe( + new RemediationObservation(Floor, TimeSpan.Zero, OtherHash, 1000), + RegressedHash, + baseline, + Floor, + Window); + + Assert.Equal(RemediationObservationVerdict.Inconclusive, result.Verdict); + } + } + + /* ---------------- the invariants ---------------- */ + + /// + /// The force is offered for EXACTLY ONE verdict, checked over every verdict the enum has rather than + /// case by case — so a verdict added later without a decision about the force fails here instead of + /// inheriting whichever arm it was written next to. + /// + [Fact] + public void ExactlyOneVerdictOffersTheForce() + { + var offering = Enum.GetValues() + .Where(OffersForce) + .ToList(); + + Assert.Equal(new[] { RemediationObservationVerdict.RegressedPlanReturned }, offering); + + static bool OffersForce(RemediationObservationVerdict verdict) => verdict switch + { + /* Derived from the machine's OWN output, not from a table retyped here: each verdict is + reproduced by an observation that reaches it, and the value's ForceOffered is read back. A + retyped table would agree with itself forever. */ + RemediationObservationVerdict.StillObserving => + Observe(0, TimeSpan.Zero, OtherHash, null).ForceOffered, + RemediationObservationVerdict.OptimizerRecovered => + Observe(Floor, TimeSpan.Zero, OtherHash, Baseline * 0.2).ForceOffered, + RemediationObservationVerdict.RegressedPlanReturned => + Observe(Floor, TimeSpan.Zero, RegressedHash, Baseline).ForceOffered, + RemediationObservationVerdict.Worse => + Observe(Floor, TimeSpan.Zero, OtherHash, Baseline * 2).ForceOffered, + RemediationObservationVerdict.Inconclusive => + Observe(Floor, TimeSpan.Zero, OtherHash, Baseline).ForceOffered, + _ => throw new InvalidOperationException( + $"verdict {verdict} has no reproducing observation in this test — decide whether it " + + "offers the force and add one"), + }; + } + + /// + /// Every verdict the machine can reach maps to a journal decision string, and the one that cannot be + /// journaled throws rather than inventing one. An in-progress observation is not a decision, and a + /// caller journaling it has a bug that a friendly fallback string would hide. + /// + [Fact] + public void EveryTerminalVerdictHasADecisionString_AndTheNonTerminalOneThrows() + { + foreach (var verdict in Enum.GetValues()) + { + if (verdict == RemediationObservationVerdict.StillObserving) + { + Assert.Throws( + () => OperatorRemediationFlow.DecisionFor(verdict)); + continue; + } + + var decision = OperatorRemediationFlow.DecisionFor(verdict); + Assert.False(string.IsNullOrWhiteSpace(decision)); + } + + /* The strings are distinct: two verdicts sharing one would make the journal unable to tell them + apart, which is the one thing the journal is for. */ + var decisions = Enum.GetValues() + .Where(v => v != RemediationObservationVerdict.StillObserving) + .Select(OperatorRemediationFlow.DecisionFor) + .ToList(); + + Assert.Equal(decisions.Count, decisions.Distinct(StringComparer.Ordinal).Count()); + } + + /// + /// The flow's "better" bar and the self-review's net-benefit bar are the same number, so an eviction + /// judged a recovery and a force judged worth keeping cannot disagree about what better means. Pinned + /// against the setting rather than the literal both happen to equal today. + /// + [Fact] + public void TheFlowAndTheSelfReviewShareOneDefinitionOfBetter() + { + Assert.Equal( + ForcePlanBotSettings.Default.NetBenefitRatio, + OperatorRemediationFlow.MaterialChangeRatio); + } +} diff --git a/Darling/Darling.Tests/OperatorRemediationGateTests.cs b/Darling/Darling.Tests/OperatorRemediationGateTests.cs new file mode 100644 index 0000000000..bc80d3e900 --- /dev/null +++ b/Darling/Darling.Tests/OperatorRemediationGateTests.cs @@ -0,0 +1,376 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; +using System.Linq; +using System.Reflection; +using System.Text.RegularExpressions; +using PerformanceMonitor.Analysis; +using Xunit; + +namespace Darling.Tests; + +/// +/// The #2138 phase-1 arming gate. Four contracts, each of which the surface exists to keep: +/// +/// a server with no remediation credential gets NO surface — not a disabled one; +/// the gate reads the existing structured_remediation verdict and asks no evidence +/// question of its own, so the PSP never-auto-force contract is inherited rather than re-implemented; +/// a disagreement between the verdict's two halves REFUSES; +/// evict-first degrading always carries a NAMED reason — never silently. +/// +/// +/// Targets are built as real s and projected through +/// FactRemediation.BuildStructuredRemediation wherever the subject is eligibility, so these cases +/// travel the same path an MCP consumer's verdict does. A hand-built +/// appears only where the subject IS a hand-built object. +/// +public sealed class OperatorRemediationGateTests +{ + private static StructuredForcePlanTarget Verdict(bool psp = false, string? replicaRole = null) + { + var target = new ForcePlanTarget( + Database: "orders", + QueryId: 42, + PlanId: 7, + BestPlanHash: "0x1111111111111111", + LatestPlanHash: "0x2222222222222222", + LatestCpuPerExecUs: 50000, + BestCpuPerExecUs: 5000, + RegressionFactor: 10.0, + ReplicaRole: replicaRole, + ParameterSensitivityCoFired: psp); + + var action = new RemediationAction( + FactKey: "PLAN_REGRESSION", + Action: "force", + Targets: new List { target }); + + var structured = FactRemediation.BuildStructuredRemediation(action); + Assert.NotNull(structured); + return structured!.ForcePlanTargets.Single(); + } + + private static readonly EvictCapability FullyCapable = new(StatementSupported: true, true); + + /* ---------------- 1. no credential, no surface ---------------- */ + + [Fact] + public void AServerWithNoRemediationCredential_GetsNoSurface() + { + Assert.Null(OperatorRemediationGate.SurfaceFor( + Verdict(), remediationCredentialConfigured: false, FullyCapable)); + } + + /// + /// The discriminating half of the test above. Without this, the null could come from anything in the + /// target — the assertion would pass against a gate that never returns a surface at all. + /// + [Fact] + public void TheSameTargetWithACredential_DoesGetASurface() + { + var surface = OperatorRemediationGate.SurfaceFor( + Verdict(), remediationCredentialConfigured: true, FullyCapable); + + Assert.NotNull(surface); + Assert.True(surface!.EvictFirstOffered); + Assert.Null(surface.EvictUnavailableReason); + } + + /// + /// The absence is expressed by the RETURN TYPE, and that is the mechanism rather than a convention: + /// a nullable return has nothing for a view to bind a command to, while an object carrying + /// Enabled = false is one IsEnabled binding away from a greyed-out button with a + /// tooltip. Read through so a change to a non-nullable return — + /// the shape that would make a disabled control the easy thing to write — fails here. + /// + [Fact] + public void TheSurfaceIsExpressedAsANullableReturn_NotAnEnabledFlag() + { + var method = typeof(OperatorRemediationGate).GetMethod(nameof(OperatorRemediationGate.SurfaceFor)); + Assert.NotNull(method); + + var nullability = new NullabilityInfoContext().Create(method!.ReturnParameter); + Assert.Equal(NullabilityState.Nullable, nullability.ReadState); + + /* And the surface type itself must carry no enabled/disabled member. The nullable return is the + whole mechanism; a bool beside it would give a view a second, contradictable answer. */ + var offenders = typeof(OperatorRemediationSurface) + .GetProperties() + .Select(p => p.Name) + .Where(n => + n.Contains("Enabled", StringComparison.OrdinalIgnoreCase) || + n.Contains("Disabled", StringComparison.OrdinalIgnoreCase) || + n.Contains("Visible", StringComparison.OrdinalIgnoreCase)) + /* EvictFirstOffered is about which LEVER, not whether the surface exists, so it is not an + offender — matched by name so this stays a name test rather than a judgement call. */ + .Where(n => !string.Equals(n, nameof(OperatorRemediationSurface.EvictFirstOffered), StringComparison.Ordinal)) + .ToList(); + + Assert.Empty(offenders); + } + + /* ---------------- 2. the verdict object is the gate ---------------- */ + + /// + /// The PSP never-auto-force contract, arriving through the projection rather than re-derived: the + /// gate never looks at ParameterSensitivityCoFired, it looks at Eligible/Blockers, + /// and the projection is what turns the flag into those. + /// + [Fact] + public void AParameterSensitiveTarget_GetsNoSurface_EvenFullyArmed() + { + var verdict = Verdict(psp: true); + + /* Proof the case is the one intended — a blocker-free verdict would make the assertion below + pass for the wrong reason. */ + Assert.False(verdict.Eligible); + Assert.Contains(verdict.Blockers, b => b == "parameter_sensitivity_cofired"); + + Assert.Null(OperatorRemediationGate.SurfaceFor( + verdict, remediationCredentialConfigured: true, FullyCapable)); + } + + [Fact] + public void ASecondaryReplicaTarget_GetsNoSurface_EvenFullyArmed() + { + var verdict = Verdict(replicaRole: "Secondary"); + + Assert.False(verdict.Eligible); + Assert.Contains(verdict.Blockers, b => b == "secondary_replica_evidence"); + + Assert.Null(OperatorRemediationGate.SurfaceFor( + verdict, remediationCredentialConfigured: true, FullyCapable)); + } + + /// + /// The gate must not have an opinion of its own. Every blocker + /// FactRemediation.ForcePlanBlockers can produce has to suppress the surface, including ones + /// added after this test was written — so the cases are derived from the projection's own output over + /// the evidence shapes that produce blockers, not from a list retyped here. + /// + [Fact] + public void EveryBlockerTheProjectionCanProduce_SuppressesTheSurface() + { + var blocking = new[] + { + Verdict(psp: true), + Verdict(replicaRole: "Secondary"), + Verdict(replicaRole: "Geo Secondary"), + Verdict(psp: true, replicaRole: "Secondary"), + }; + + /* Positive control: the shapes really do produce blockers, so an empty-blockers projection + cannot make this pass by making every case eligible. */ + Assert.Empty(blocking.Where(v => v.Blockers.Count == 0)); + + foreach (var verdict in blocking) + { + Assert.Null(OperatorRemediationGate.SurfaceFor( + verdict, remediationCredentialConfigured: true, FullyCapable)); + } + } + + /* ---------------- 3. disagreement refuses ---------------- */ + + /// + /// A hand-built or deserialized verdict whose two halves disagree must refuse, in BOTH directions. + /// The projection can never emit either shape (it defines Eligible as + /// blockers.Count == 0), which is exactly why the gate reading only one of them would look + /// correct forever while being one wire format away from arming a blocked target. + /// + [Theory] + [InlineData(true, "parameter_sensitivity_cofired")] + [InlineData(false, null)] + public void AVerdictWhoseHalvesDisagree_GetsNoSurface(bool eligible, string? blocker) + { + var blockers = blocker is null ? Array.Empty() : new[] { blocker }; + var handBuilt = new StructuredForcePlanTarget( + "orders", 42, 7, "0x2222222222222222", "0x1111111111111111", null, + Eligible: eligible, + Blockers: blockers, + Evidence: new StructuredForcePlanEvidence(10.0, 50000, 5000, blocker is not null), + ForceSql: "force", + UnforceSql: "unforce", + VerifySql: "verify"); + + Assert.Null(OperatorRemediationGate.SurfaceFor( + handBuilt, remediationCredentialConfigured: true, FullyCapable)); + } + + /* ---------------- 4. the degrade always names a reason ---------------- */ + + [Fact] + public void AnEnginePlatformWithoutTheStatement_DegradesToForceOnly_WithThatNamedReason() + { + var surface = OperatorRemediationGate.SurfaceFor( + Verdict(), + remediationCredentialConfigured: true, + new EvictCapability(StatementSupported: false, CredentialHoldsAlterServerState: true)); + + Assert.NotNull(surface); + Assert.False(surface!.EvictFirstOffered); + Assert.Equal(EvictDegradeReasons.UnsupportedOnPlatform, surface.EvictUnavailableReason); + } + + [Fact] + public void ACredentialWithoutAlterServerState_DegradesToForceOnly_WithThatNamedReason() + { + var surface = OperatorRemediationGate.SurfaceFor( + Verdict(), + remediationCredentialConfigured: true, + new EvictCapability(StatementSupported: true, CredentialHoldsAlterServerState: false)); + + Assert.NotNull(surface); + Assert.False(surface!.EvictFirstOffered); + Assert.Equal(EvictDegradeReasons.PermissionDenied, surface.EvictUnavailableReason); + } + + [Fact] + public void AnUnprobedCapability_DegradesWithItsOwnReason_RatherThanGuessing() + { + var surface = OperatorRemediationGate.SurfaceFor( + Verdict(), remediationCredentialConfigured: true, EvictCapability.Unprobed); + + Assert.NotNull(surface); + Assert.False(surface!.EvictFirstOffered); + Assert.Equal(EvictDegradeReasons.CapabilityUnknown, surface.EvictUnavailableReason); + } + + /// + /// On a platform with no statement, the answer must NOT be the permission reason — that one sends an + /// operator to ask a cloud provider for a grant that would change nothing. Pinned separately from the + /// arms above because it is a precedence claim, and precedence is what a later reordering breaks. + /// + [Fact] + public void PlatformOutranksPermission_SoNoOneIsSentToAskForAGrantThatCannotHelp() + { + Assert.Equal( + EvictDegradeReasons.UnsupportedOnPlatform, + OperatorRemediationGate.EvictUnavailableReason( + new EvictCapability(StatementSupported: false, CredentialHoldsAlterServerState: false))); + + Assert.Equal( + EvictDegradeReasons.UnsupportedOnPlatform, + OperatorRemediationGate.EvictUnavailableReason( + new EvictCapability(StatementSupported: false, CredentialHoldsAlterServerState: null))); + } + + /// + /// THE never-silently contract, over every capability shape there is: a surface that does not offer + /// evict-first always carries a reason, and one that does never carries a stale one. Exhaustive rather + /// than case-by-case because the failure mode is a shape nobody enumerated — a new + /// field would add shapes here and the null-reason arm would catch the + /// one that fell through. + /// + [Fact] + public void NoCapabilityShapeCanDegradeWithoutANamedReason() + { + var shapes = + from supported in new[] { true, false } + from granted in new bool?[] { true, false, null } + select new EvictCapability(supported, granted); + + var checked_ = 0; + foreach (var shape in shapes) + { + var surface = OperatorRemediationGate.SurfaceFor( + Verdict(), remediationCredentialConfigured: true, shape); + Assert.NotNull(surface); + checked_++; + + if (surface!.EvictFirstOffered) + { + Assert.Null(surface.EvictUnavailableReason); + } + else + { + Assert.False( + string.IsNullOrWhiteSpace(surface.EvictUnavailableReason), + $"evict-first is unavailable for {shape} with no named reason — the degrade must " + + "never be silent"); + } + } + + /* The loop really ran over every shape: 2 x 3. A comprehension that produced nothing would + otherwise pass this test by asserting about no cases. */ + Assert.Equal(6, checked_); + } + + /// + /// Exactly one capability shape offers evict-first. Stated as a count so a change that quietly widens + /// the grant — say, treating an unprobed capability as permitted — fails here rather than showing up + /// as an unexpected DBCC FREEPROCCACHE attempt against a server. + /// + [Fact] + public void ExactlyOneCapabilityShapeOffersEvictFirst() + { + var offering = + (from supported in new[] { true, false } + from granted in new bool?[] { true, false, null } + let capability = new EvictCapability(supported, granted) + where OperatorRemediationGate.EvictUnavailableReason(capability) is null + select capability).ToList(); + + var only = Assert.Single(offering); + Assert.True(only.StatementSupported); + Assert.True(only.CredentialHoldsAlterServerState == true); + } + + /// + /// The capability probe is a SELECT. It runs as the remediation credential against a production + /// server, so the one thing it must never be is a statement that changes something — asserted against + /// the shipped constant rather than a copy, and by naming the statements it must not contain rather + /// than by matching a shape a rewrite would slip past. + /// + [Fact] + public void TheCapabilityProbeIsReadOnly() + { + var sql = OperatorRemediationGate.AlterServerStateProbeSql; + + Assert.StartsWith("SELECT ", sql, StringComparison.Ordinal); + + /* Quoted literals are stripped, then the scan is by WORD BOUNDARY rather than substring — and both + refinements were forced by this test failing on the real statement, twice, for reasons that were + the scan's fault rather than the SQL's. First the permission NAME 'ALTER SERVER STATE' matched + as a DDL keyword; then the output alias has_alter_server_state did, because a substring scan + cannot tell an identifier from a statement. A scan with either flaw has to be silenced to ship, + and a silenced scan guards nothing. */ + var withoutLiterals = Regex.Replace(sql, "'[^']*'", "''", RegexOptions.CultureInvariant); + Assert.Contains("''", withoutLiterals, StringComparison.Ordinal); + + foreach (var forbidden in new[] + { "DBCC", "FREEPROCCACHE", "EXEC", "EXECUTE", "ALTER", "UPDATE", "DELETE", "INSERT", "DROP", "MERGE", "TRUNCATE" }) + { + Assert.DoesNotMatch( + new Regex($@"\b{forbidden}\b", RegexOptions.IgnoreCase | RegexOptions.CultureInvariant), + withoutLiterals); + } + + /* Positive control for the scan itself: it must fire on a statement that really does write, or + the loop above is asserting nothing about anything. Underscore-joined and quoted forms stay + clear, which is exactly the discrimination the two failures above were about. */ + var writeShape = "SELECT 1; DBCC FREEPROCCACHE(0x00);"; + Assert.Matches(new Regex(@"\bDBCC\b", RegexOptions.IgnoreCase), writeShape); + Assert.DoesNotMatch(new Regex(@"\bALTER\b", RegexOptions.IgnoreCase), "SELECT has_alter_server_state = 1;"); + + /* One statement. A trailing terminator is house style; a second one would let a read-only-looking + probe carry anything after it. */ + Assert.Equal(1, sql.Count(c => c == ';')); + Assert.EndsWith(";", sql.TrimEnd(), StringComparison.Ordinal); + + /* It asks the server-scope question, not a database-scope one: has_perms_by_name's first two + arguments must both be NULL or it answers about the current database instead, which would + report a grant an eviction cannot use. */ + Assert.Contains("has_perms_by_name(NULL, NULL, 'ALTER SERVER STATE')", sql, StringComparison.Ordinal); + + /* House style: every output column aliased, so the answer is not called has_perms_by_name. */ + Assert.Contains("has_alter_server_state =", sql, StringComparison.Ordinal); + } +} diff --git a/Darling/Darling.Tests/PlanForceActionStoreTests.cs b/Darling/Darling.Tests/PlanForceActionStoreTests.cs index 5a96126665..c889d5cd5d 100644 --- a/Darling/Darling.Tests/PlanForceActionStoreTests.cs +++ b/Darling/Darling.Tests/PlanForceActionStoreTests.cs @@ -209,6 +209,42 @@ await store.JournalAsync(Record(now, await store.GetPendingReviewsAsync(TestServerId, now, ct), r => r.ActionId == orphanIntent); + /* 8. OWN-FORCES-ONLY, as a predicate rather than a circumstance (V113, #2138 phase 1). + Until an operator could write to this table, the property held because the bot was the + only writer; it does not hold by itself any more. An operator's succeeded live force is + shaped EXACTLY like a bot force the read would return — same action, same outcome, same + server, no closing row — so the only thing that can keep it out is the actor filter, and + nothing else in this scenario could make the assertion pass. + + The bot-actored twin is journaled in the same breath as the discriminating control: without + it, an actor filter that matched NOTHING (a typo in the value, a filter on the wrong column) + would satisfy the first assertion perfectly. */ + var operatorForce = await store.JournalAsync(Record(now.AddMinutes(-90), + action: PgPlanForceActionStore.ActionForce, decision: PgPlanForceActionStore.ActionForce, + reasons: "", outcome: PgPlanForceActionStore.OutcomeSucceeded, + mode: PgPlanForceActionStore.ModeLive, + actor: PgPlanForceActionStore.ActorOperator), ct); + var botForce = await store.JournalAsync(Record(now.AddMinutes(-90), + action: PgPlanForceActionStore.ActionForce, decision: PgPlanForceActionStore.ActionForce, + reasons: "", outcome: PgPlanForceActionStore.OutcomeSucceeded, + mode: PgPlanForceActionStore.ModeLive, + actor: PgPlanForceActionStore.ActorBot), ct); + + var reviewable = await store.GetPendingReviewsAsync(TestServerId, now, ct); + Assert.DoesNotContain(reviewable, r => r.ActionId == operatorForce); + Assert.Contains(reviewable, r => r.ActionId == botForce); + + /* And the actor round-trips on the read, so a consumer can tell the two apart in the audit + trail rather than only the review read being able to. A column written but never read back + is a column that drifts. */ + var audited = await store.GetRecentActionsAsync(TestServerId, now.AddDays(-1), 200, ct); + Assert.Equal( + PgPlanForceActionStore.ActorOperator, + Assert.Single(audited.Where(r => r.ActionId == operatorForce)).Actor); + Assert.Equal( + PgPlanForceActionStore.ActorBot, + Assert.Single(audited.Where(r => r.ActionId == botForce)).Actor); + bodySucceeded = true; } finally @@ -225,7 +261,11 @@ private static PlanForceActionRecord Record( string reasons, string outcome, string mode = PgPlanForceActionStore.ModeDryRun, - long? relatedActionId = null) => new( + long? relatedActionId = null, + /* Defaults to the bot because every pre-V113 scenario in this file is a bot scenario, so the + existing cases keep asserting exactly what they asserted. The operator value is passed + explicitly, only by the tests whose subject is the actor. */ + string actor = PgPlanForceActionStore.ActorBot) => new( ActionId: 0, ActionTimeUtc: timeUtc, ServerId: TestServerId, @@ -235,6 +275,7 @@ private static PlanForceActionRecord Record( PlanId: 7, Action: action, Mode: mode, + Actor: actor, Decision: decision, Reasons: reasons, RegressionFactor: 12.5, diff --git a/Darling/Darling.Tests/PostgresTargetConfigTests.cs b/Darling/Darling.Tests/PostgresTargetConfigTests.cs index 4b033bb3f7..e95bfa0522 100644 --- a/Darling/Darling.Tests/PostgresTargetConfigTests.cs +++ b/Darling/Darling.Tests/PostgresTargetConfigTests.cs @@ -353,6 +353,12 @@ derivation. It belongs in this list rather than beside Password's exemption beca for a WRITE authorization would be a silent no-op on every seeded box (#2254). It round-trips through the registry read, which is exactly what this test checks. */ ("PlanForceBotEnabled", "plan_force_bot_enabled"), + /* V113 (#2138 phase 1): the per-server remediation credential. Unlike PlanForceBotEnabled + above, these two ARE settable from darling.json — an arm state belongs to the registry, but a + credential has to be suppliable on a container install with no viewer to type one into. So + they round-trip in both directions and belong here rather than beside Password's exemption. */ + ("RemediationUsername", "remediation_username"), + ("RemediationEncryptedPassword", "remediation_encrypted_password"), }; /* Password is the deliberate exception: a plaintext dev password is never persisted, and is diff --git a/Darling/Darling.Tests/RegisteredServerSettingDriftTests.cs b/Darling/Darling.Tests/RegisteredServerSettingDriftTests.cs index 253de0c140..887f1670b4 100644 --- a/Darling/Darling.Tests/RegisteredServerSettingDriftTests.cs +++ b/Darling/Darling.Tests/RegisteredServerSettingDriftTests.cs @@ -875,7 +875,17 @@ public void EveryPerServerDarlingJsonKeyIsEitherComparedOrDeliberatelyExcluded() /* The credential, and nothing else. A file entry legitimately carries a reference or a dev plaintext password against a store row holding a DPAPI blob, which is the supported shape rather than drift — and comparing a secret is how one reaches a log line. */ - var excluded = new HashSet(System.StringComparer.Ordinal) { "password", "encryptedPassword" }; + /* V113 (#2138 phase 1) adds a SECOND credential, excluded for the same two reasons plus a third + that is specific to it. Same two: a file entry legitimately carries a reference against a store + row holding a blob, and comparing a secret is how one reaches a log line. The third: a drift + report is what triggers a disconnect-and-reconnect of the MONITORING connection, and the + remediation credential has nothing to do with that connection — reporting it would tear down + collection on a server because someone rotated a credential collection never uses. */ + var excluded = new HashSet(System.StringComparer.Ordinal) + { + "password", "encryptedPassword", + "remediationUsername", "remediationEncryptedPassword", + }; var keys = typeof(MonitoredServer) .GetProperties(BindingFlags.Public | BindingFlags.Instance) @@ -947,7 +957,26 @@ a field label that is not a real key sends the operator to edit something that i unknown.Length == 0, "the drift report names a field that is not a darling.json per-server key: " + string.Join(", ", unknown)); - /* And that the exclusion list has not quietly grown past the credential. */ - Assert.Equal(new[] { "encryptedPassword", "password" }, excluded.OrderBy(k => k).ToArray()); + /* And that the exclusion list has not quietly grown past the credential. The literal IS the + mechanism: an exclusion is only legitimate with an argument, and the argument belongs in the + comment above beside the key, so growing this list has to show up in a diff. V113 (#2138 phase 1) + added the remediation credential's two keys, which is why it is four rather than two. */ + Assert.Equal( + new[] { "encryptedPassword", "password", "remediationEncryptedPassword", "remediationUsername" }, + excluded.OrderBy(k => k).ToArray()); + + /* The property behind the literal, so this is not purely a frozen list that the next lane bumps + by reflex: only a CREDENTIAL key may be excluded. That is what stops the exclusion becoming the + easy way out for any field whose comparison is inconvenient — excluding trustServerCertificate, + the field #2552 actually reported, would fail here rather than passing with a bumped literal. */ + foreach (var key in excluded) + { + Assert.Matches( + new System.Text.RegularExpressions.Regex( + "(?:password|username)$", + System.Text.RegularExpressions.RegexOptions.IgnoreCase + | System.Text.RegularExpressions.RegexOptions.CultureInvariant), + key); + } } } diff --git a/Darling/Darling.Tests/RemediationCredentialRungTests.cs b/Darling/Darling.Tests/RemediationCredentialRungTests.cs new file mode 100644 index 0000000000..5a562cec1a --- /dev/null +++ b/Darling/Darling.Tests/RemediationCredentialRungTests.cs @@ -0,0 +1,234 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Linq; +using PerformanceMonitor.Darling.Service; +using PerformanceMonitor.Darling.Storage; +using PerformanceMonitor.Darling.Viewer; +using Xunit; + +namespace Darling.Tests; + +/// +/// V113 (#2138 phase 1): the per-server remediation credential and the journal's actor. +/// +/// This suite also carries the TOP-RUNG guard, which moved here from +/// when V113 dethroned V112. That guard has to live with +/// whichever rung is actually top: it asserts that a store with every sentinel true maps to exactly the +/// head of the ladder, and its whole point is to catch a later rung that appends a sentinel without adding +/// its own arm — which would leave the viewer refusing a store that is perfectly current, permanently, +/// because no further upgrade changes the answer. A rung that keeps claiming the title after losing it +/// breaks every older rung's suite instead. +/// +public class RemediationCredentialRungTests +{ + internal const int RungVersion = 113; + + /// The version a store one rung behind this one reports. + private const int PreviousVersion = 112; + + /// This rung's sentinel ordinal in the viewer probe. Its OWN ordinal, which never moves. + internal const int ProbeOrdinal = 88; + + [Fact] + public void TheRungIsRegisteredAtTheTopOfADenseLadder() + { + var versions = PgMigrations.Scripts.Select(s => s.Version).ToList(); + + Assert.Equal( + "remediation-credential-and-actor", + PgMigrations.Scripts.Single(s => s.Version == RungVersion).Name); + + Assert.Equal(StorageVersion.SchemaVersion, PgMigrations.Scripts[^1].Version); + Assert.Equal(StorageVersion.SchemaVersion, versions.Max()); + Assert.Equal(RungVersion, StorageVersion.SchemaVersion); + + Assert.Equal(versions.Distinct().OrderBy(v => v), versions); + var above = versions.Where(v => v > 45).OrderBy(v => v).ToList(); + Assert.Equal(Enumerable.Range(above[0], above.Count), above); + } + + /// + /// The rung's four statements. Both ALTERs are schema-qualified for the reason every rung here is: the + /// migrate session's search_path puts collect first, so a bare name resolves wherever + /// that points rather than where the rung meant. + /// + [Fact] + public void TheRungAddsTheCredentialColumnsTheActorAndItsIndex() + { + var sql = PgMigrations.Scripts.Single(s => s.Version == RungVersion).Sql; + + Assert.Contains( + "ALTER TABLE config.config_monitored_servers", sql, StringComparison.Ordinal); + Assert.Contains( + "ADD COLUMN IF NOT EXISTS remediation_username text", sql, StringComparison.Ordinal); + Assert.Contains( + "ADD COLUMN IF NOT EXISTS remediation_encrypted_password text", sql, StringComparison.Ordinal); + Assert.Contains( + "ALTER TABLE collect.plan_force_actions", sql, StringComparison.Ordinal); + Assert.Contains( + "ADD COLUMN IF NOT EXISTS actor text NOT NULL DEFAULT 'bot'", sql, StringComparison.Ordinal); + Assert.Contains( + "idx_plan_force_actions_actor", sql, StringComparison.Ordinal); + } + + /// + /// THE reason the rung has four statements instead of three: the actor's DEFAULT is added and then + /// dropped, in that order, in the same rung. + /// + /// Added because every row that predates the column really was written by the bot, so + /// 'bot' is the only honest backfill. Dropped because leaving it in place would make an INSERT + /// that FORGETS actor silently claim to be the bot — and a bot row is the kind the self-review + /// is allowed to unforce, so the default fails in the one direction that costs. The ordering is + /// asserted by POSITION rather than by presence: both statements present in the wrong order would + /// leave the column with a default and every existing row null-violating, which is the worst of both. + /// + [Fact] + public void TheActorDefaultIsAddedForTheBackfillThenDroppedSoAForgottenInsertFails() + { + var sql = PgMigrations.Scripts.Single(s => s.Version == RungVersion).Sql; + + var added = sql.IndexOf("DEFAULT 'bot'", StringComparison.Ordinal); + var dropped = sql.IndexOf("ALTER COLUMN actor DROP DEFAULT", StringComparison.Ordinal); + + Assert.True(added >= 0, "the actor column must carry DEFAULT 'bot' so existing rows backfill honestly"); + Assert.True(dropped >= 0, "the actor DEFAULT must be dropped so an INSERT that omits it fails loudly"); + Assert.True( + added < dropped, + "the DEFAULT must be added BEFORE it is dropped — the reverse order leaves a live default and a " + + "NOT NULL column full of nulls"); + } + + /// + /// The journal's INSERT names actor. With the DEFAULT dropped this is not a style point: an + /// INSERT that omits the column raises 23502 against a live store, so the writer and the rung have to + /// agree. Read off the shipped SQL rather than a copy. + /// + [Fact] + public void TheJournalWriterNamesTheActorColumn() + { + var sql = ParitySourceLocal.ReadFile( + "Darling/PerformanceMonitor.Darling.Service/PgPlanForceActionStore.cs"); + + Assert.Contains("action, mode, actor, decision, reasons,", sql, StringComparison.Ordinal); + + /* And the read the invariant rests on filters on it. A writer that stamps the actor while the + review read ignores it would leave own-forces-only broken with every row correctly labelled. */ + Assert.Contains("pfa.actor = 'bot'", sql, StringComparison.Ordinal); + } + + /// + /// The actor is a REQUIRED record member, so a construction site that forgets it does not compile. + /// Asserted through reflection on the primary constructor because that is the property being claimed — + /// a defaulted parameter would let a new writer journal as whichever actor the default named, and one + /// of the two is the one the review may act on. + /// + [Fact] + public void TheActorIsRequiredOnTheRecord_SoTheCompilerEnumeratesCallSites() + { + var constructor = typeof(PlanForceActionRecord) + .GetConstructors() + .OrderByDescending(c => c.GetParameters().Length) + .First(); + + var actor = Assert.Single( + constructor.GetParameters().Where(p => p.Name == "Actor")); + + Assert.False( + actor.HasDefaultValue, + "PlanForceActionRecord.Actor must have no default: a construction site that omits it has to " + + "fail to compile rather than silently pick an actor."); + + /* Positive control for the reflection: the record really does carry defaulted parameters elsewhere + in the codebase's style, so "HasDefaultValue is false" is a fact about this parameter rather + than about the reflection call always returning false. ReplicaRole on ForcePlanTarget is the + nearest example of the appended-with-a-default pattern this one deliberately does not follow. */ + var replicaRole = Assert.Single( + typeof(PerformanceMonitor.Analysis.ForcePlanTarget) + .GetConstructors() + .OrderByDescending(c => c.GetParameters().Length) + .First() + .GetParameters() + .Where(p => p.Name == "ReplicaRole")); + Assert.True(replicaRole.HasDefaultValue); + } + + /// + /// The connect-time gate. A COLUMN sentinel on actor, because both objects this rung touches + /// already exist — the registry since V17, the journal since V107 — so table existence cannot separate + /// the rungs, and the actor is the one the own-forces-only invariant turns on. Deliberately not a + /// credential column: those are the secret and non-secret halves of one optional feature, and a probe + /// line naming one reads as though the viewer needed to see it. + /// + /// The top-rung guard. Every sentinel true must map to exactly this version, or the + /// viewer refuses a fully-migrated store forever. The all-true argument list is built by reflection so + /// the arity tracks the signature: the literal-true form silently defaults a newly added sentinel to + /// false and maps one version low, which is the failure this guard exists for. + /// + [Fact] + public void TheProbeAsksForTheActorColumn_AndMapsAFullyMigratedStoreToThisRung() + { + Assert.Contains( + "table_name = 'plan_force_actions' AND column_name = 'actor'", + ViewerDataService.StoreSchemaProbeSql, StringComparison.Ordinal); + + var viewer = ParitySourceLocal.ReadFile( + "Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.cs"); + Assert.Contains($"reader.GetBoolean({ProbeOrdinal})", viewer, StringComparison.Ordinal); + Assert.Contains("hasRemediationCredentialAndActor", viewer, StringComparison.Ordinal); + + Assert.Equal(StorageVersion.SchemaVersion, ViewerDataService.RequiredStoreSchemaVersion); + + var method = typeof(ViewerDataService) + .GetMethod("MapProbedSchemaVersion", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static)!; + var arity = method.GetParameters().Length; + + /* As the TOP rung, this sentinel is the last one — and that equality is what catches a later rung + appending a sentinel without adding its own arm. When a later rung lands, this clause moves to + it and becomes ProbeOrdinal < arity here. */ + Assert.Equal(arity - 1, ProbeOrdinal); + + var all = Enumerable.Repeat((object)true, arity).ToArray(); + Assert.Equal(StorageVersion.SchemaVersion, (int)method.Invoke(null, all)!); + + /* One rung behind: every sentinel EXCEPT this one must report 112. Without this the arm above + could be satisfied by an unconditional return and nothing would notice. */ + var allButMine = Enumerable.Repeat((object)true, arity).ToArray(); + allButMine[ProbeOrdinal] = false; + Assert.Equal(PreviousVersion, (int)method.Invoke(null, allButMine)!); + } + + /// + /// The two new registry columns are classified in the viewer's column ACL, and on the right sides. The + /// live security gate already asserts the union covers the table; what it cannot assert is that the + /// SECRET one landed in the secret list rather than being waved through to fix a failing build. + /// + [Fact] + public void TheCredentialColumnsAreClassifiedWithTheSecretOnTheSecretSide() + { + var acl = Assert.Single( + DarlingManagedRoles.ViewerRestrictedConfigTables + .Where(t => t.Table == "config_monitored_servers")); + + Assert.Contains("remediation_username", acl.NonSecretColumns); + Assert.Contains("remediation_encrypted_password", acl.SecretColumns); + Assert.DoesNotContain("remediation_encrypted_password", acl.NonSecretColumns); + } +} + +/// +/// Reads a repo file by a path relative to the repo root. A local helper because +/// Lite.Tests.ParitySource is in the other test assembly and Darling.Tests.RepoFile takes +/// path segments; both resolve the same root the same way. +/// +internal static class ParitySourceLocal +{ + internal static string ReadFile(string relativePath) => + RepoFile.ReadRepoFile(relativePath.Split('/')); +} diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingConfig.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingConfig.cs index 10bb19590f..46e16d8508 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingConfig.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingConfig.cs @@ -798,10 +798,16 @@ public sealed class ForcePlanBotFileConfig [JsonPropertyName("finalReviewMinutes")] public int FinalReviewMinutes { get; set; } = 1440; - /// Executions required before a checkpoint judges cost. + /// Executions required before a checkpoint judges cost, and the executions limb of the + /// operator flow's post-eviction observation window. [JsonPropertyName("minReviewExecutions")] public int MinReviewExecutions { get; set; } = 25; + /// The elapsed limb of the post-eviction observation window, in minutes — the operator flow + /// observes until whichever of the two limbs fires first. + [JsonPropertyName("observationWindowMinutes")] + public int ObservationWindowMinutes { get; set; } = 30; + /// Post-force cpu/exec must be at or below this fraction of the baseline, or the review unforces. [JsonPropertyName("netBenefitRatio")] public double NetBenefitRatio { get; set; } = 0.75; @@ -820,6 +826,7 @@ public PerformanceMonitor.Analysis.ForcePlanBotSettings ToSettings() => FirstReviewMinutes = FirstReviewMinutes, FinalReviewMinutes = FinalReviewMinutes, MinReviewExecutions = MinReviewExecutions, + ObservationWindowMinutes = ObservationWindowMinutes, NetBenefitRatio = NetBenefitRatio, }.Normalize(); } @@ -1759,6 +1766,52 @@ public sealed class MonitoredServer [JsonIgnore] public bool PlanForceBotEnabled { get; set; } + /// + /// The REMEDIATION credential's login name (config_monitored_servers.remediation_username) — the + /// second, per-server, opt-in identity a #2138 phase-1 action runs as. + /// + /// The monitoring credential is never used for a write, ever. That promise is stated in + /// both READMEs and in the MCP instructions, and operators grant against it, so the write travels on + /// its own identity or it does not travel. There is no fallback: null here means this server has no + /// phase-1 surface at all, which is the whole arming model — see + /// for why the absence is + /// a null credential rather than an enabled flag. + /// + /// Presence IS the auth mode. Deliberately no remediationAuth sibling: an + /// integrated remediation identity would be the service account, which is the monitoring identity, + /// which is exactly what this exists to keep read-only. So a remediation credential is always SQL auth + /// when set, and there is no third state to resolve wrongly. + /// + /// Settable from the file, unlike — and the difference is + /// deliberate. That flag is an ARM STATE, so a file knob would be a silent no-op on a seeded box + /// (#2254). This is a CREDENTIAL, and the container/compose deploy has no viewer to type one into; the + /// env:/file: reference path is the only way to arm a Linux install at all. It is still + /// only read at seed time like every other credential field here. + /// + [JsonPropertyName("remediationUsername")] + public string? RemediationUsername { get; set; } + + /// + /// The remediation credential's DPAPI-LocalMachine blob, base64 — produced by the same + /// --encrypt-password as , and resolvable as an + /// env:/file: reference by the same DarlingSecretSource. There is deliberately no + /// plaintext sibling of this one (no counterpart to ): the dev-convenience + /// plaintext slot exists because a wrong monitoring password fails a read, and a wrong remediation + /// password fails a write to a production server. + /// + [JsonPropertyName("remediationEncryptedPassword")] + public string? RemediationEncryptedPassword { get; set; } + + /// + /// Whether this server is armed for operator-initiated remediation: BOTH halves of the credential are + /// present. A one-sided credential is not a weaker arm, it is a misconfiguration — so it reads as + /// unarmed rather than as something to attempt and fail at against a production server. + /// + [JsonIgnore] + public bool HasRemediationCredential => + !string.IsNullOrWhiteSpace(RemediationUsername) && + !string.IsNullOrWhiteSpace(RemediationEncryptedPassword); + /// /// This server's server_id: the stored value when there is one, otherwise derived from /// . diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingManagedRoles.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingManagedRoles.cs index d7a1ee15b4..a0ec3b24e8 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingManagedRoles.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingManagedRoles.cs @@ -125,8 +125,20 @@ public static class DarlingManagedRoles fail-closed gate is why it must be named here: unclassified stays invisible to `viewer` and the live security test fails until someone decides which side it is on. */ "plan_force_bot_enabled", + /* V113 (#2138 phase 1): the remediation credential's LOGIN NAME. Non-secret on the same + reasoning as `username` two lines up — a login name is not a credential, and it is the + only column that can answer "which identity would a remediation run as", which an + operator has to be able to audit without holding the secret. It is also how the viewer + learns a server is armed at all: the phase-1 surface exists when this is non-null, so a + `viewer` seat that could not read it would see no surface on an armed server. */ + "remediation_username", }, - SecretColumns: new[] { "encrypted_password" }), + /* remediation_encrypted_password is the same kind of thing as encrypted_password beside it: a + DPAPI blob whose whole purpose is to authenticate a WRITE to a monitored server, so if + anything in this table is secret it is. Named explicitly rather than left unclassified + because unclassified is only invisible until someone "fixes" the failing security gate by + adding the column to whichever list is nearer. */ + SecretColumns: new[] { "encrypted_password", "remediation_encrypted_password" }), new ViewerSecretTableAcl( "config_command", diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingSecrets.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingSecrets.cs index 869840c23f..83a4cb9d19 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingSecrets.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingSecrets.cs @@ -129,4 +129,62 @@ public static string ResolvePassword(MonitoredServer server, out bool usedPlaint throw new InvalidOperationException( $"Server '{server.DisplayName}' uses sql auth but has neither encryptedPassword nor password."); } + + /// + /// Resolves a server's REMEDIATION credential password (#2138 phase 1) — the second, opt-in identity a + /// write to a monitored server travels on. Same two shapes accepts, minus + /// the plaintext one. + /// + /// Returns null for an unarmed server rather than throwing, and that asymmetry with + /// is the point. A missing monitoring password is a misconfiguration — the + /// operator declared sql auth and left the secret out — so it throws. A missing remediation password is + /// the SHIPPED STATE of every server: nothing has gone wrong, this server simply has no phase-1 + /// surface. Making it throw would turn the normal case into an exception, and an exception in the normal + /// case is a thing callers learn to swallow. + /// + /// There is no plaintext arm. 's dev-convenience slot has no + /// remediation counterpart: a wrong monitoring password fails a read, and a wrong remediation password + /// fails a write against a production server, so the convenience is not worth the same money. An + /// env:/file: reference is still accepted — a pointer is not a secret, and it is the only + /// way to arm an install with no DPAPI (the #2087 reasoning). + /// + /// A DPAPI failure DOES throw, through the same text the + /// other three surfaces use: an armed server whose blob will not decrypt is a real fault, and it is + /// exactly the one a viewer-on-a-different-PC produces. + /// + public static string? ResolveRemediationPassword(MonitoredServer server) + { + if (server is null) + { + throw new ArgumentNullException(nameof(server)); + } + + /* Both halves or nothing — HasRemediationCredential, not just the blob. A blob with no username + cannot build a connection string, and resolving its secret first would decrypt a credential to + then discover it is unusable. */ + if (!server.HasRemediationCredential) + { + return null; + } + + var blob = server.RemediationEncryptedPassword!; + + if (DarlingSecretSource.IsReference(blob)) + { + return DarlingSecretSource.Resolve( + blob, $"servers['{server.DisplayName}'].remediationEncryptedPassword"); + } + + try + { + return Unprotect(blob); + } + catch (CryptographicException ex) + { + throw new InvalidOperationException( + DescribeDecryptFailure($"the stored REMEDIATION password for server '{server.DisplayName}' " + + "(servers[].remediationEncryptedPassword)"), + ex); + } + } } diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingServerConnector.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingServerConnector.cs index 9f0b1ab194..1ea72c2a65 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingServerConnector.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingServerConnector.cs @@ -179,9 +179,60 @@ public static string ResolveConnectionString(MonitoredServer config, ILogger? lo } } + WarnIfRemediationCredentialIsInert(config, logger); + return MonitoredServerConnection.BuildConnectionString(config, password); } + /// + /// Says out loud that an armed remediation credential does nothing in this build (#2138 phase 1). + /// + /// Why this exists. V113 accepts a per-server remediation credential and nothing in this + /// build consumes it — the write path is its own change. An operator who has entered one believes the + /// server is armed, and the failure mode of a knob that silently does nothing is at its worst when what + /// it claims to gate is a write to a production server. So the same discipline #2745 applied to its + /// all-gates-open force (journal it as WITHHELD rather than quietly downgrading it) applies here: the + /// credential is accepted, stored, resolvable, and announced as inert. + /// + /// Once per connect rather than once per sweep: connects are rare, so this cannot become the + /// every-60-seconds log line #2255 was about. A one-sided credential is reported separately, because + /// "you configured half of one" and "this build cannot use it yet" send an operator to different + /// places — the first is a mistake to fix now, the second is a wait. + /// + private static void WarnIfRemediationCredentialIsInert(MonitoredServer config, ILogger? logger) + { + var username = !string.IsNullOrWhiteSpace(config.RemediationUsername); + var secret = !string.IsNullOrWhiteSpace(config.RemediationEncryptedPassword); + + if (username ^ secret) + { + logger?.LogWarning( + "Server '{Server}' has only one half of a remediation credential ({Half} is set, the other is not), so it counts as unarmed. Both remediationUsername and remediationEncryptedPassword are required.", + config.DisplayName, + username ? "remediationUsername" : "remediationEncryptedPassword"); + return; + } + + if (username && secret) + { + /* "yet" is only true where the capability is coming. BuildRemediationConnectionString + throws for a PostgreSQL target, and plan-force remediation is a Query Store concept, + so on Postgres this credential is inert PERMANENTLY rather than pending. One message + for both would promise an operator a future that engine does not have. */ + if (config.IsPostgres) + { + logger?.LogWarning( + "Server '{Server}' has a remediation credential, but plan-force remediation is SQL Server-only (it forces a Query Store plan), so nothing on a PostgreSQL target will ever use it. Remove it, or move it to the SQL Server registration it was meant for.", + config.DisplayName); + return; + } + + logger?.LogInformation( + "Server '{Server}' has a remediation credential, but this build ships no remediation write path (#2138 phase 1 is the credential seam, the journal's actor and the decision logic). Nothing will use it yet, and the monitoring credential remains read-only.", + config.DisplayName); + } + } + /* The PostgreSQL detection query. Deliberately built only from surfaces a pg_monitor-grade login can read on Amazon Aurora, verified against live 16.11 and 17.7 clusters: diff --git a/Darling/PerformanceMonitor.Darling.Service/MonitoredServerConnection.cs b/Darling/PerformanceMonitor.Darling.Service/MonitoredServerConnection.cs index a81c894dcf..447928d04c 100644 --- a/Darling/PerformanceMonitor.Darling.Service/MonitoredServerConnection.cs +++ b/Darling/PerformanceMonitor.Darling.Service/MonitoredServerConnection.cs @@ -67,6 +67,98 @@ public static string BuildConnectionString(MonitoredServer server, string? resol return builder.ConnectionString; } + /// + /// The connection string for a #2138 phase-1 REMEDIATION action: the same posture as + /// , on the server's second, opt-in remediation identity. + /// + /// A separate function rather than a parameter on the one above, because the two differ in + /// ways a boolean would have to be read correctly at every call site: the credential is always SQL auth + /// (there is no integrated arm to fall into), the identity is not the monitoring one, and the + /// application name is deliberately different. A useRemediationCredential: true flag on the main + /// builder would put the write identity one mistyped argument away from every collector. + /// + /// The ApplicationName is the audit trail on the server's side. A DBA reading + /// sys.dm_exec_sessions during an incident needs to be able to tell this apart from the + /// collection connections, and "the monitoring tool" answering for both would make the one connection + /// that can change a plan indistinguishable from the forty that cannot. It is also what makes an XE + /// or Profiler filter on this feature possible at all. + /// + /// No MARS, and a tighter command budget. The collection loop wants multiple active result + /// sets; a remediation runs one statement. And 60 seconds is a collection budget — a + /// sp_query_store_force_plan that has not returned in 30 is not going to, and holding the + /// connection longer only delays the journal row that says so. + /// + /// Postgres targets throw rather than returning something: Query Store plan forcing is a SQL + /// Server concept, so a PostgreSQL target reaching here is a caller that skipped the engine gate, and + /// the #2213 lesson is that the failure has to be loud at the boundary rather than an + /// ArgumentException from a driver parsing the wrong keyword shape. + /// + public static string BuildRemediationConnectionString( + MonitoredServer server, string resolvedRemediationPassword) + { + if (server is null) + { + throw new ArgumentNullException(nameof(server)); + } + + if (string.IsNullOrWhiteSpace(resolvedRemediationPassword)) + { + throw new ArgumentException( + "A remediation connection requires the remediation credential's password.", + nameof(resolvedRemediationPassword)); + } + + if (server.IsPostgres) + { + throw new InvalidOperationException( + $"Server '{server.DisplayName}' is a PostgreSQL target; Query Store plan remediation is a " + + "SQL Server concept and this call site should have been engine-gated."); + } + + if (!server.HasRemediationCredential) + { + throw new InvalidOperationException( + $"Server '{server.DisplayName}' has no remediation credential, so no remediation connection " + + "can be built for it."); + } + + var builder = new SqlConnectionStringBuilder + { + DataSource = server.Host, + InitialCatalog = string.IsNullOrWhiteSpace(server.Database) ? "master" : server.Database, + ApplicationName = RemediationApplicationName, + ConnectTimeout = 15, + CommandTimeout = 30, + TrustServerCertificate = server.TrustServerCertificate, + MultipleActiveResultSets = false, + /* Never ReadOnly, whatever the server's ReadOnlyIntent says. A remediation connection routed to + a read-only secondary by an intent hint would fail the write with a message about the replica + rather than about the routing, and the monitoring entry's intent is a COLLECTION preference + that has no business steering a write. */ + ApplicationIntent = ApplicationIntent.ReadWrite, + MultiSubnetFailover = server.MultiSubnetFailover, + }; + + builder.Encrypt = server.EncryptMode?.Trim().ToUpperInvariant() switch + { + "STRICT" => SqlConnectionEncryptOption.Strict, + "OPTIONAL" => SqlConnectionEncryptOption.Optional, + _ => SqlConnectionEncryptOption.Mandatory, + }; + + builder.UserID = server.RemediationUsername; + builder.Password = resolvedRemediationPassword; + + return builder.ConnectionString; + } + + /// + /// The ApplicationName a remediation connection presents. A named constant because it is the + /// only thing a DBA on the far end can filter on, so it is a documented interface rather than a string + /// — and because a test can then assert the remediation and collection connections do not share it. + /// + public const string RemediationApplicationName = "PerformanceMonitorDarling-Remediation"; + /// /// The PostgreSQL equivalent, keeping the same posture the SQL Server path establishes: a /// 15-second connect budget, a 60-second command budget, TLS required unless explicitly relaxed, diff --git a/Darling/PerformanceMonitor.Darling.Service/PgPlanForceActionStore.cs b/Darling/PerformanceMonitor.Darling.Service/PgPlanForceActionStore.cs index 50194f02b8..c051a3d659 100644 --- a/Darling/PerformanceMonitor.Darling.Service/PgPlanForceActionStore.cs +++ b/Darling/PerformanceMonitor.Darling.Service/PgPlanForceActionStore.cs @@ -16,7 +16,15 @@ namespace PerformanceMonitor.Darling.Service; -/// One journal row (V107 collect.plan_force_actions). Timestamps naive UTC. +/// +/// One journal row (V107 collect.plan_force_actions, V113's actor). Timestamps naive UTC. +/// +/// has NO default, deliberately. It is the column the own-forces-only invariant +/// rests on (see ), so a construction site that +/// forgets it must not compile — a defaulted member would let a new writer silently journal as whichever +/// actor the default named, and one of the two possible defaults is the one whose forces the bot is allowed +/// to take back. Requiring it means the compiler enumerates every site instead of a reviewer having to. +/// public sealed record PlanForceActionRecord( long ActionId, DateTime ActionTimeUtc, @@ -27,6 +35,7 @@ public sealed record PlanForceActionRecord( long PlanId, string Action, string Mode, + string Actor, string Decision, string Reasons, double RegressionFactor, @@ -74,6 +83,28 @@ readable in psql and a future Lite twin shares the exact values. */ public const string ModeDryRun = "dry_run"; public const string ModeLive = "live"; + /* WHO decided (V113, #2138 phase 1) — orthogonal to Mode, which is HOW. The bot can be live or dry + run; an operator is always live, because a human clicking a button in a shadow-mode rehearsal is not + a thing the design has. + + This pair is load-bearing rather than descriptive. GetPendingReviewsAsync' own-forces-only property + was structural while the bot was the only writer to this table; phase 1 makes an operator a writer, + and the standing house rule is that operator-placed forces are NEVER touched by the bot's + self-review. The filter on ActorBot is what keeps that true, so these two strings are a contract: + a third actor added later must be considered against that read explicitly, not just spelled. */ + public const string ActorBot = "bot"; + public const string ActorOperator = "operator"; + + /* The operator flow's own actions (#2138 phase 1). Deliberately DISTINCT verbs from ActionForce rather + than a force row with an operator actor, because they are different acts with different follow-ups: + an eviction pins nothing and is owed no review, while a force pins a plan and is. Sharing the verb + would make "how many plans has this tool pinned on this server" un-answerable by a COUNT. */ + public const string ActionEvict = "evict"; + + /// The post-eviction observation's verdict row — the journal's record of what the window saw. + /// Its decision is one of OperatorRemediationFlow's decision strings. + public const string ActionObserve = "observe"; + public const string OutcomeLogged = "logged"; public const string OutcomeAttempting = "attempting"; public const string OutcomeSucceeded = "succeeded"; @@ -104,10 +135,10 @@ public async Task JournalAsync(PlanForceActionRecord record, CancellationT await using var command = new NpgsqlCommand(@" INSERT INTO collect.plan_force_actions ( action_time, server_id, server_name, database_name, query_id, plan_id, - action, mode, decision, reasons, + action, mode, actor, decision, reasons, regression_factor, latest_cpu_per_exec_us, best_cpu_per_exec_us, replica_role, parameter_sensitivity_cofired, outcome, detail, related_action_id) -VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18) +VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19) RETURNING action_id", connection) { CommandTimeout = ServiceCommandDeadlines.PostAnalysisForcePlanSeconds, @@ -123,6 +154,7 @@ INSERT INTO collect.plan_force_actions ( command.Parameters.AddWithValue(record.PlanId); command.Parameters.AddWithValue(record.Action); command.Parameters.AddWithValue(record.Mode); + command.Parameters.AddWithValue(record.Actor); command.Parameters.AddWithValue(record.Decision); command.Parameters.AddWithValue(record.Reasons); command.Parameters.AddWithValue(record.RegressionFactor); @@ -149,6 +181,21 @@ INSERT INTO collect.plan_force_actions ( /// failed forces for the query inside the failure-memory window: force rows whose outcome /// is failed, plus unforce rows the self-review issued (not_net_benefit / force_failing). /// + /// + /// Deliberately NOT filtered by actor, unlike + /// . The two reads want opposite things and the asymmetry is the + /// design, not an oversight — so do not "fix" it for symmetry. That read authorizes the bot to UNDO + /// something, and undoing another actor's work is the thing forbidden. These three aggregates RESTRAIN + /// the bot, and every limb restrains it correctly by counting an operator's rows too: a query a human + /// touched two hours ago is exactly a query the bot should stay off; an operator's force spends real + /// blast radius on that server; and an operator's force that would not stick is real evidence the next + /// one will not either. Filtering here would make the bot MORE willing to act the more a human already + /// had, which is backwards. + /// + /// The budget limb counts would_force/force and so does not see an operator's + /// evict rows. That is intended: an eviction pins nothing and the optimizer may recover on its + /// own, so it does not carry a force's blast radius, and spending the bot's force budget on one would + /// let a cheap reversible act lock out an expensive irreversible one. /// public async Task GetQueryHistoryAsync( int serverId, string database, long queryId, ForcePlanBotSettings settings, DateTime nowUtc, CancellationToken ct) @@ -229,8 +276,12 @@ AND pfa.action_time > $5 /// happened, and the state machine closes it either way (still forced → a real review; /// not forced → no_longer_forced). /// - /// OWN-FORCES-ONLY is structural here — the read starts from rows this bot journaled, so an - /// operator's hand-placed force can never surface as something to unforce. + /// OWN-FORCES-ONLY is a PREDICATE here, not a structural property — actor = 'bot' (V113). + /// It was structural while the bot was this table's only writer: the read started from rows the bot + /// journaled, so an operator's hand-placed force could not surface. #2138 phase 1 makes an operator a + /// writer to the same table, and an operator's succeeded live force is shaped exactly like a bot force + /// this read returns — same action, same outcome, no closing row. The filter is now the only thing + /// keeping the guarantee, so do not remove it for looking redundant. /// /// Specced here, consumed by the write path (#2731): phase 1 places no live force, so this /// read is provably empty in this build. It lands with the journal rather than with the bot arm @@ -244,12 +295,19 @@ public async Task> GetPendingReviewsAsync( await using var connection = await _postgres.OpenConnectionAsync(ct); await using var command = new NpgsqlCommand(@" SELECT pfa.action_id, pfa.action_time, pfa.server_id, pfa.server_name, pfa.database_name, - pfa.query_id, pfa.plan_id, pfa.action, pfa.mode, pfa.decision, pfa.reasons, + pfa.query_id, pfa.plan_id, pfa.action, pfa.mode, pfa.actor, pfa.decision, pfa.reasons, pfa.regression_factor, pfa.latest_cpu_per_exec_us, pfa.best_cpu_per_exec_us, pfa.replica_role, pfa.parameter_sensitivity_cofired, pfa.outcome, pfa.detail, pfa.related_action_id FROM collect.plan_force_actions AS pfa WHERE pfa.server_id = $1 AND pfa.action = 'force' +/* OWN-FORCES-ONLY, as a predicate rather than a circumstance (V113). Until phase 1 this read's + comment could say the property was structural because the bot was the only writer to the table. + An operator is now a writer to the same table, so without this line the bot's self-review would + find an operator's force, judge it against evidence it never saw, and unforce it — breaking the + standing rule that operator-placed forces are never touched, in the one direction nobody would + notice until a plan they pinned by hand quietly stopped being pinned. */ +AND pfa.actor = 'bot' AND (pfa.outcome = 'succeeded' /* An intent whose completion row exists is accounted for (succeeded rows anchor their own pending entry; failed rows need no review). Only an intent NOTHING references, past the @@ -293,7 +351,7 @@ public async Task> GetRecentActionsAsync( await using var connection = await _postgres.OpenConnectionAsync(ct); await using var command = new NpgsqlCommand(@" SELECT pfa.action_id, pfa.action_time, pfa.server_id, pfa.server_name, pfa.database_name, - pfa.query_id, pfa.plan_id, pfa.action, pfa.mode, pfa.decision, pfa.reasons, + pfa.query_id, pfa.plan_id, pfa.action, pfa.mode, pfa.actor, pfa.decision, pfa.reasons, pfa.regression_factor, pfa.latest_cpu_per_exec_us, pfa.best_cpu_per_exec_us, pfa.replica_role, pfa.parameter_sensitivity_cofired, pfa.outcome, pfa.detail, pfa.related_action_id FROM collect.plan_force_actions AS pfa @@ -329,14 +387,15 @@ ORDER BY pfa.action_time DESC PlanId: reader.GetInt64(6), Action: reader.GetString(7), Mode: reader.GetString(8), - Decision: reader.GetString(9), - Reasons: reader.GetString(10), - RegressionFactor: Convert.ToDouble(reader.GetValue(11), CultureInfo.InvariantCulture), - LatestCpuPerExecUs: Convert.ToDouble(reader.GetValue(12), CultureInfo.InvariantCulture), - BestCpuPerExecUs: Convert.ToDouble(reader.GetValue(13), CultureInfo.InvariantCulture), - ReplicaRole: reader.IsDBNull(14) ? null : reader.GetString(14), - ParameterSensitivityCoFired: reader.GetBoolean(15), - Outcome: reader.GetString(16), - Detail: reader.IsDBNull(17) ? null : reader.GetString(17), - RelatedActionId: reader.IsDBNull(18) ? null : reader.GetInt64(18)); + Actor: reader.GetString(9), + Decision: reader.GetString(10), + Reasons: reader.GetString(11), + RegressionFactor: Convert.ToDouble(reader.GetValue(12), CultureInfo.InvariantCulture), + LatestCpuPerExecUs: Convert.ToDouble(reader.GetValue(13), CultureInfo.InvariantCulture), + BestCpuPerExecUs: Convert.ToDouble(reader.GetValue(14), CultureInfo.InvariantCulture), + ReplicaRole: reader.IsDBNull(15) ? null : reader.GetString(15), + ParameterSensitivityCoFired: reader.GetBoolean(16), + Outcome: reader.GetString(17), + Detail: reader.IsDBNull(18) ? null : reader.GetString(18), + RelatedActionId: reader.IsDBNull(19) ? null : reader.GetInt64(19)); } diff --git a/Darling/PerformanceMonitor.Darling.Service/PlanForceBot.cs b/Darling/PerformanceMonitor.Darling.Service/PlanForceBot.cs index e4dd432088..e8d5f89a29 100644 --- a/Darling/PerformanceMonitor.Darling.Service/PlanForceBot.cs +++ b/Darling/PerformanceMonitor.Darling.Service/PlanForceBot.cs @@ -234,6 +234,10 @@ private PlanForceActionRecord BuildRecord( PlanId: target.PlanId, Action: action, Mode: _settings.DryRun ? PgPlanForceActionStore.ModeDryRun : PgPlanForceActionStore.ModeLive, + /* Always the bot: this class IS the bot, and it has no operator-driven arm. Stamped as a + constant rather than passed in so there is no argument to get wrong — and it is what makes + GetPendingReviewsAsync' actor filter meet rows it can actually match. */ + Actor: PgPlanForceActionStore.ActorBot, Decision: action, Reasons: string.Join(",", reasons), RegressionFactor: target.RegressionFactor, diff --git a/Darling/PerformanceMonitor.Darling.Service/StoreConfigProvider.cs b/Darling/PerformanceMonitor.Darling.Service/StoreConfigProvider.cs index 18f309264d..1963dd6db0 100644 --- a/Darling/PerformanceMonitor.Darling.Service/StoreConfigProvider.cs +++ b/Darling/PerformanceMonitor.Darling.Service/StoreConfigProvider.cs @@ -1089,8 +1089,9 @@ Viewer deletion (Stage 3) is never resurrected by a re-seed. */ INSERT INTO config_monitored_servers ( server_id, name, host, database, auth, username, encrypted_password, encrypt_mode, trust_server_certificate, read_only_intent, multi_subnet_failover, excluded_databases, - monthly_cost_usd, capture_plans, alert_delivery_mode_override, engine, port, is_enabled, plan_force_bot_enabled, created_at, modified_at) -VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, NULL, $14, $16, $17, TRUE, FALSE, $15, $15) + monthly_cost_usd, capture_plans, alert_delivery_mode_override, engine, port, is_enabled, plan_force_bot_enabled, + remediation_username, remediation_encrypted_password, created_at, modified_at) +VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, NULL, $14, $16, $17, TRUE, FALSE, $18, $19, $15, $15) ON CONFLICT (server_id) DO NOTHING", connection) { CommandTimeout = ServiceCommandDeadlines.BootstrapSeconds }; /* THE ALLOCATION SITE. A darling.json entry has no StoredServerId, so this is the derivation — and this is where it is minted and made permanent. When new rows stop being hash-keyed @@ -1121,6 +1122,15 @@ single parse in MonitoredServer.TargetEngine stays the only place that interpret engine — a non-default port dropped here would connect to 5432 and fail with an error naming the right host. */ command.Parameters.AddWithValue(server.Port); + /* V113 (#2138 phase 1): the remediation credential, if darling.json carried one. Seeded for + the same reason as the monitoring credential and NOT for the reason plan_force_bot_enabled is + hardcoded FALSE two lines up: that is an arm STATE the registry owns after seeding, while + this is a credential, and a container install with no viewer has no other way to supply one. + Nullable with no default, so a darling.json without these keys seeds two NULLs and the server + is simply unarmed. There is no plaintext fallback to merge at read time: RemediationPassword + does not exist, deliberately. */ + AddNullableText(command, server.RemediationUsername); + AddNullableText(command, server.RemediationEncryptedPassword); await command.ExecuteNonQueryAsync(ct); } } @@ -1468,7 +1478,7 @@ twelve downstream sites re-derived it from the mutable columns instead. */ using var command = new NpgsqlCommand(@" SELECT name, host, database, auth, username, encrypted_password, encrypt_mode, trust_server_certificate, read_only_intent, multi_subnet_failover, excluded_databases, monthly_cost_usd, alert_delivery_mode_override, - engine, port, server_id, plan_force_bot_enabled + engine, port, server_id, plan_force_bot_enabled, remediation_username, remediation_encrypted_password FROM config_monitored_servers WHERE is_enabled = TRUE ORDER BY name", connection) { CommandTimeout = ServiceCommandDeadlines.SerialLoopSeconds }; using var reader = await command.ExecuteReaderAsync(ct); @@ -1522,6 +1532,12 @@ private static MonitoredServer BuildServerFromRow(NpgsqlDataReader reader, Darli DBNull guard is for a store mid-migration — and it reads as NOT opted in, because a write authorization must fail CLOSED when the store cannot answer. */ PlanForceBotEnabled = !reader.IsDBNull(16) && reader.GetBoolean(16), + /* V113 (#2138 phase 1): the per-server remediation credential. Nullable in the table with no + default, so DBNull is the EXPECTED reading for every server nobody has armed — which is + every server until an operator types one in. A null here is not a degraded state to warn + about; it is the shipped state, and it means this server has no phase-1 surface. */ + RemediationUsername = reader.IsDBNull(17) ? null : reader.GetString(17), + RemediationEncryptedPassword = reader.IsDBNull(18) ? null : reader.GetString(18), }; if (server.UsesSqlAuth && string.IsNullOrWhiteSpace(server.EncryptedPassword)) diff --git a/Darling/PerformanceMonitor.Darling.Service/darling.sample.json b/Darling/PerformanceMonitor.Darling.Service/darling.sample.json index dcf59dd538..958242f096 100644 --- a/Darling/PerformanceMonitor.Darling.Service/darling.sample.json +++ b/Darling/PerformanceMonitor.Darling.Service/darling.sample.json @@ -130,6 +130,21 @@ "encryptedPassword": "", "trustServerCertificate": false, "encryptMode": "Mandatory", + // OPTIONAL second credential, for #2138 operator-initiated remediation only. The monitoring + // credential above stays READ-ONLY forever and is never used for a write; a remediation action + // travels on this identity or it does not travel. Omit both keys (the default) and this server + // simply has no remediation surface -- there is nothing to disable and nothing to explain. + // Both halves are required; one alone counts as unarmed and the service says so at connect. + // Least privilege: ALTER on each database you intend to force a plan in, and -- only if you want + // evict-first -- ALTER SERVER STATE, which is what targeted DBCC FREEPROCCACHE(plan_handle) needs. + // That second grant is the expensive one and is genuinely optional: without it the flow degrades + // to force-only with a named reason rather than silently. The service PROBES for it with a + // read-only has_perms_by_name check instead of guessing per platform, and Azure SQL Database has + // no DBCC FREEPROCCACHE at all, which no grant changes. + // Same blob format as encryptedPassword (--encrypt-password on THIS machine, or an env:/file: + // reference). There is deliberately no plaintext variant of this one. + "remediationUsername": "", + "remediationEncryptedPassword": "", "excludedDatabases": [ "StageDb" ] } ], diff --git a/Darling/PerformanceMonitor.Darling.Storage/PgMigrations.cs b/Darling/PerformanceMonitor.Darling.Storage/PgMigrations.cs index 038f11baae..3284df3d96 100644 --- a/Darling/PerformanceMonitor.Darling.Storage/PgMigrations.cs +++ b/Darling/PerformanceMonitor.Darling.Storage/PgMigrations.cs @@ -169,6 +169,7 @@ here costs a fresh-through-this-rung store nothing and rung 54's own copy no-ops new Migration(110, "collection-log-fetch-phase-sums", V110Sql), new Migration(111, "store-log-self-monitoring", V111Sql), new Migration(112, "collector-stall-wait-probes", V112Sql), + new Migration(113, "remediation-credential-and-actor", V113Sql), }; /// @@ -2788,6 +2789,62 @@ the second sample - the one that proves the degradation is not confined to a sin CREATE INDEX IF NOT EXISTS idx_collector_stall_probes_time ON collect.collector_stall_probes(server_id, probe_time);"; + /// + /// V113 — the per-server REMEDIATION CREDENTIAL, and the journal's actor (#2138 phase 1). + /// + /// The credential. The monitoring credential stays read-only forever; that promise is + /// load-bearing (the MCP instructions and both READMEs state it, and operators grant against it), so a + /// write to a monitored server cannot travel on it. remediation_username / + /// remediation_encrypted_password are a SECOND, per-server, opt-in credential in the same shape + /// as username / encrypted_password beside them — same DPAPI-LocalMachine blob, same + /// env:/file: reference support, produced by the same --encrypt-password. Both + /// nullable with NO default and NO fallback: a server whose remediation columns are null has no + /// phase-1 surface at all, which is why the absence is expressed as a null credential rather than as an + /// enabled boolean — a boolean invites a disabled control, and a missing credential is supposed + /// to be unrenderable rather than explained. + /// + /// Deliberately NOT reusing the auth column's vocabulary: a remediation credential is + /// always SQL auth when present (an integrated remediation identity would be the service account, + /// which is the monitoring identity, which is the thing this exists to avoid). Presence of the username + /// IS the auth mode, so there is no third state to get wrong. + /// + /// The actor. V107's journal was written when the bot was the only possible writer, and + /// PgPlanForceActionStore.GetPendingReviewsAsync rests on that: its own-forces-only property is + /// documented as structural because "the read starts from rows this bot journaled". Phase 1 makes an + /// OPERATOR a writer to the same table, and that sentence stops being true the moment it does — the + /// bot's self-review would pick up an operator's force and take it back, breaking the standing house + /// rule that operator-placed forces are never touched. actor restores the invariant as data: the + /// review read filters actor = 'bot', so own-forces-only is a predicate on the table rather than + /// a property of who happened to be able to write to it. + /// + /// The DEFAULT is added and then dropped, and that is the point. Every existing row was + /// written by the bot, so DEFAULT 'bot' backfills them correctly and is the only honest value + /// for rows that predate the column. Leaving the default in place afterwards would make an INSERT that + /// forgets actor silently claim to be the bot — the one direction that matters, because a bot row + /// is the kind the review is allowed to unforce. Dropping it makes that INSERT fail loudly instead. The + /// C# side reinforces it: PlanForceActionRecord.Actor is a required member, so a construction + /// site that omits it does not compile. + /// + private const string V113Sql = @" +ALTER TABLE config.config_monitored_servers + ADD COLUMN IF NOT EXISTS remediation_username text; + +ALTER TABLE config.config_monitored_servers + ADD COLUMN IF NOT EXISTS remediation_encrypted_password text; + +ALTER TABLE collect.plan_force_actions + ADD COLUMN IF NOT EXISTS actor text NOT NULL DEFAULT 'bot'; + +ALTER TABLE collect.plan_force_actions + ALTER COLUMN actor DROP DEFAULT; + +/* The review read is (server_id, actor, action) with an ordering on action_time, and it is the read the + own-forces-only invariant rests on, so it gets its own index rather than riding + idx_plan_force_actions_time - which leads with server_id but knows nothing about the actor and would + make every pending-review scan read the operator's rows to discard them. */ +CREATE INDEX IF NOT EXISTS idx_plan_force_actions_actor + ON collect.plan_force_actions(server_id, actor, action, action_time);"; + /// /// V105 — collect.collector_cost, the tool's own per-collector cost on the monitored servers /// (#2674). NOT a collector: it is INTERNAL self-telemetry, written by the worker's hourly self-metrics diff --git a/Darling/PerformanceMonitor.Darling.Storage/StorageVersion.cs b/Darling/PerformanceMonitor.Darling.Storage/StorageVersion.cs index 92015677c7..7b0cbe4860 100644 --- a/Darling/PerformanceMonitor.Darling.Storage/StorageVersion.cs +++ b/Darling/PerformanceMonitor.Darling.Storage/StorageVersion.cs @@ -16,5 +16,5 @@ namespace PerformanceMonitor.Darling.Storage; /// public static class StorageVersion { - public const int SchemaVersion = 112; + public const int SchemaVersion = 113; } diff --git a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.cs b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.cs index 35dae931f8..7dc0b63fb3 100644 --- a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.cs +++ b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.cs @@ -726,7 +726,8 @@ existence cannot separate the rungs and information_schema.columns is the only s EXISTS (SELECT 1 FROM information_schema.columns WHERE table_name = 'collection_log' AND column_name = 'drain_last_read_ms'), EXISTS (SELECT 1 FROM information_schema.columns WHERE table_name = 'collection_log' AND column_name = 'plan_fetch_target_ms'), EXISTS (SELECT 1 FROM information_schema.tables WHERE table_name = 'store_log_events'), - EXISTS (SELECT 1 FROM information_schema.tables WHERE table_name = 'collector_stall_probes')"; + EXISTS (SELECT 1 FROM information_schema.tables WHERE table_name = 'collector_stall_probes'), + EXISTS (SELECT 1 FROM information_schema.columns WHERE table_name = 'plan_force_actions' AND column_name = 'actor')"; /// The store schema version this viewer build requires — the highest migration it knows /// (). The connect-time gate blocks a store below this. @@ -748,7 +749,7 @@ existence cannot separate the rungs and information_schema.columns is the only s await using var reader = await command.ExecuteReaderAsync(cancellationToken); if (await reader.ReadAsync(cancellationToken)) { - return MapProbedSchemaVersion(reader.GetBoolean(0), reader.GetBoolean(1), reader.GetBoolean(2), reader.GetBoolean(3), reader.GetBoolean(4), reader.GetBoolean(5), reader.GetBoolean(6), reader.GetBoolean(7), reader.GetBoolean(8), reader.GetBoolean(9), reader.GetBoolean(10), reader.GetBoolean(11), reader.GetBoolean(12), reader.GetBoolean(13), reader.GetBoolean(14), reader.GetBoolean(15), reader.GetBoolean(16), reader.GetBoolean(17), reader.GetBoolean(18), reader.GetBoolean(19), reader.GetBoolean(20), reader.GetBoolean(21), reader.GetBoolean(22), reader.GetBoolean(23), reader.GetBoolean(24), reader.GetBoolean(25), reader.GetBoolean(26), reader.GetBoolean(27), reader.GetBoolean(28), reader.GetBoolean(29), reader.GetBoolean(30), reader.GetBoolean(31), reader.GetBoolean(32), reader.GetBoolean(33), reader.GetBoolean(34), reader.GetBoolean(35), reader.GetBoolean(36), reader.GetBoolean(37), reader.GetBoolean(38), reader.GetBoolean(39), reader.GetBoolean(40), reader.GetBoolean(41), reader.GetBoolean(42), reader.GetBoolean(43), reader.GetBoolean(44), reader.GetBoolean(45), reader.GetBoolean(46), reader.GetBoolean(47), reader.GetBoolean(48), reader.GetBoolean(49), reader.GetBoolean(50), reader.GetBoolean(51), reader.GetBoolean(52), reader.GetBoolean(53), reader.GetBoolean(54), reader.GetBoolean(55), reader.GetBoolean(56), reader.GetBoolean(57), reader.GetBoolean(58), reader.GetBoolean(59), reader.GetBoolean(60), reader.GetBoolean(61), reader.GetBoolean(62), reader.GetBoolean(63), reader.GetBoolean(64), reader.GetBoolean(65), reader.GetBoolean(66), reader.GetBoolean(67), reader.GetBoolean(68), reader.GetBoolean(69), reader.GetBoolean(70), reader.GetBoolean(71), reader.GetBoolean(72), reader.GetBoolean(73), reader.GetBoolean(74), reader.GetBoolean(75), reader.GetBoolean(76), reader.GetBoolean(77), reader.GetBoolean(78), reader.GetBoolean(79), reader.GetBoolean(80), reader.GetBoolean(81), reader.GetBoolean(82), reader.GetBoolean(83), reader.GetBoolean(84), reader.GetBoolean(85), reader.GetBoolean(86), reader.GetBoolean(87)); + return MapProbedSchemaVersion(reader.GetBoolean(0), reader.GetBoolean(1), reader.GetBoolean(2), reader.GetBoolean(3), reader.GetBoolean(4), reader.GetBoolean(5), reader.GetBoolean(6), reader.GetBoolean(7), reader.GetBoolean(8), reader.GetBoolean(9), reader.GetBoolean(10), reader.GetBoolean(11), reader.GetBoolean(12), reader.GetBoolean(13), reader.GetBoolean(14), reader.GetBoolean(15), reader.GetBoolean(16), reader.GetBoolean(17), reader.GetBoolean(18), reader.GetBoolean(19), reader.GetBoolean(20), reader.GetBoolean(21), reader.GetBoolean(22), reader.GetBoolean(23), reader.GetBoolean(24), reader.GetBoolean(25), reader.GetBoolean(26), reader.GetBoolean(27), reader.GetBoolean(28), reader.GetBoolean(29), reader.GetBoolean(30), reader.GetBoolean(31), reader.GetBoolean(32), reader.GetBoolean(33), reader.GetBoolean(34), reader.GetBoolean(35), reader.GetBoolean(36), reader.GetBoolean(37), reader.GetBoolean(38), reader.GetBoolean(39), reader.GetBoolean(40), reader.GetBoolean(41), reader.GetBoolean(42), reader.GetBoolean(43), reader.GetBoolean(44), reader.GetBoolean(45), reader.GetBoolean(46), reader.GetBoolean(47), reader.GetBoolean(48), reader.GetBoolean(49), reader.GetBoolean(50), reader.GetBoolean(51), reader.GetBoolean(52), reader.GetBoolean(53), reader.GetBoolean(54), reader.GetBoolean(55), reader.GetBoolean(56), reader.GetBoolean(57), reader.GetBoolean(58), reader.GetBoolean(59), reader.GetBoolean(60), reader.GetBoolean(61), reader.GetBoolean(62), reader.GetBoolean(63), reader.GetBoolean(64), reader.GetBoolean(65), reader.GetBoolean(66), reader.GetBoolean(67), reader.GetBoolean(68), reader.GetBoolean(69), reader.GetBoolean(70), reader.GetBoolean(71), reader.GetBoolean(72), reader.GetBoolean(73), reader.GetBoolean(74), reader.GetBoolean(75), reader.GetBoolean(76), reader.GetBoolean(77), reader.GetBoolean(78), reader.GetBoolean(79), reader.GetBoolean(80), reader.GetBoolean(81), reader.GetBoolean(82), reader.GetBoolean(83), reader.GetBoolean(84), reader.GetBoolean(85), reader.GetBoolean(86), reader.GetBoolean(87), reader.GetBoolean(88)); } return null; @@ -773,7 +774,7 @@ existence cannot separate the rungs and information_schema.columns is the only s /// is unit-tested without a live store; any schema bump past the newest arm trips the pinning test that keeps /// this in step with . /// - internal static int MapProbedSchemaVersion(bool hasConfigControlPlane, bool hasAlertDeliveryOverride, bool hasAnalysisState, bool hasAlertTuningKnobs, bool hasDefaultTraceEvents, bool hasIndexObjectStatsLatestIndex, bool hasCollectionLogHypertableOrPlainPg, bool hasJobHistory, bool hasAgentStatus, bool hasGenericWebhook, bool hasDeadlocksDatabaseName, bool hasQueryStoreReplicaRole, bool hasLongQueryCompletions, bool hasWebDashboardConfig, bool hasCustomViews, bool hasServerTags, bool hasConnectionRefireKnobs = false, bool hasAgCollectors = false, bool hasAgAlertKnobs = false, bool hasAgLatencyColumns = false, bool hasAgDisconnectRefire = false, bool hasPayloadDimensions = false, bool hasDimFloorIndexes = false, bool hasBlockingWaitThreshold = false, bool hasQueryStoreIntervalIdentity = false, bool hasPagerDutyWebhook = false, bool hasPagerDutyProxy = false, bool hasCollectorState = false, bool hasPlanCorrection = false, bool hasPvsStats = false, bool hasPvsPressureKnobs = false, bool hasDatabaseStateAlert = false, bool hasServerTagColour = false, bool hasQueryStatsHostObject = false, bool hasFindingDrillDown = false, bool hasStoreMetrics = false, bool hasPlanDimGzip = false, bool hasSelfAlertKnobs = false, bool hasJobMetricsColumns = false, bool hasJobCadenceKnob = false, bool hasBackfillSwitch = false, bool hasCollectorMemoryKnobs = false, bool hasDatabaseStateEdgeMemory = false, bool hasIncidentOccurrences = false, bool hasPlanXmlCompressionKnob = false, bool hasMonitoredServerEngine = false, bool hasPgBlockingEdges = false, bool hasQueryStorePlanMap = false, bool hasPgStatementText = false, bool hasQueryStoreText = false, bool hasPlanContentRetentionKnob = false, bool hasQueryStoreHealth = false, bool hasQueryStoreTextHash = false, bool hasComposeTimeoutKnob = false, bool hasFileGrowthAlert = false, bool hasCollectionLogFanoutRollup = false, bool hasTempDbMaxSize = false, bool hasServerEngineKind = false, bool hasPgDatabaseStats = false, bool hasPgIndexUsageStats = false, bool hasPgTableBloatStats = false, bool hasPgSessionStates = false, bool hasPgPlanCaptureReadiness = false, bool hasPgWriteStats = false, bool hasPgExtensionAvailability = false, bool hasPgLockStats = false, bool hasPgColumnStats = false, bool hasPgReplicationStats = false, bool hasPgBufferUsage = false, bool hasPgIndexBloat = false, bool hasPgPerDatabaseAttribution = false, bool hasPgWaitSampling = false, bool hasPgKernelStats = false, bool hasPgPredicateStats = false, bool hasPgPlanCapture = false, bool hasPgMajorVersion = false, bool hasPg18IoBytes = false, bool hasPgServerConfig = false, bool hasPgDeadlocks = false, bool hasPgDeadlockIdentity = false, bool hasCollectorCost = false, bool hasPgCpuUtilization = false, bool hasPlanForceActions = false, bool hasCollectionLogPhaseSplit = false, bool hasCollectionLogDrainForensics = false, bool hasCollectionLogFetchPhaseSums = false, bool hasStoreLogSelfMonitoring = false, bool hasCollectorStallProbes = false) + internal static int MapProbedSchemaVersion(bool hasConfigControlPlane, bool hasAlertDeliveryOverride, bool hasAnalysisState, bool hasAlertTuningKnobs, bool hasDefaultTraceEvents, bool hasIndexObjectStatsLatestIndex, bool hasCollectionLogHypertableOrPlainPg, bool hasJobHistory, bool hasAgentStatus, bool hasGenericWebhook, bool hasDeadlocksDatabaseName, bool hasQueryStoreReplicaRole, bool hasLongQueryCompletions, bool hasWebDashboardConfig, bool hasCustomViews, bool hasServerTags, bool hasConnectionRefireKnobs = false, bool hasAgCollectors = false, bool hasAgAlertKnobs = false, bool hasAgLatencyColumns = false, bool hasAgDisconnectRefire = false, bool hasPayloadDimensions = false, bool hasDimFloorIndexes = false, bool hasBlockingWaitThreshold = false, bool hasQueryStoreIntervalIdentity = false, bool hasPagerDutyWebhook = false, bool hasPagerDutyProxy = false, bool hasCollectorState = false, bool hasPlanCorrection = false, bool hasPvsStats = false, bool hasPvsPressureKnobs = false, bool hasDatabaseStateAlert = false, bool hasServerTagColour = false, bool hasQueryStatsHostObject = false, bool hasFindingDrillDown = false, bool hasStoreMetrics = false, bool hasPlanDimGzip = false, bool hasSelfAlertKnobs = false, bool hasJobMetricsColumns = false, bool hasJobCadenceKnob = false, bool hasBackfillSwitch = false, bool hasCollectorMemoryKnobs = false, bool hasDatabaseStateEdgeMemory = false, bool hasIncidentOccurrences = false, bool hasPlanXmlCompressionKnob = false, bool hasMonitoredServerEngine = false, bool hasPgBlockingEdges = false, bool hasQueryStorePlanMap = false, bool hasPgStatementText = false, bool hasQueryStoreText = false, bool hasPlanContentRetentionKnob = false, bool hasQueryStoreHealth = false, bool hasQueryStoreTextHash = false, bool hasComposeTimeoutKnob = false, bool hasFileGrowthAlert = false, bool hasCollectionLogFanoutRollup = false, bool hasTempDbMaxSize = false, bool hasServerEngineKind = false, bool hasPgDatabaseStats = false, bool hasPgIndexUsageStats = false, bool hasPgTableBloatStats = false, bool hasPgSessionStates = false, bool hasPgPlanCaptureReadiness = false, bool hasPgWriteStats = false, bool hasPgExtensionAvailability = false, bool hasPgLockStats = false, bool hasPgColumnStats = false, bool hasPgReplicationStats = false, bool hasPgBufferUsage = false, bool hasPgIndexBloat = false, bool hasPgPerDatabaseAttribution = false, bool hasPgWaitSampling = false, bool hasPgKernelStats = false, bool hasPgPredicateStats = false, bool hasPgPlanCapture = false, bool hasPgMajorVersion = false, bool hasPg18IoBytes = false, bool hasPgServerConfig = false, bool hasPgDeadlocks = false, bool hasPgDeadlockIdentity = false, bool hasCollectorCost = false, bool hasPgCpuUtilization = false, bool hasPlanForceActions = false, bool hasCollectionLogPhaseSplit = false, bool hasCollectionLogDrainForensics = false, bool hasCollectionLogFetchPhaseSums = false, bool hasStoreLogSelfMonitoring = false, bool hasCollectorStallProbes = false, bool hasRemediationCredentialAndActor = false) { /* V71 (the PostgreSQL blocking-edges rung): a table-existence sentinel and now the newest-first arm. A collector table would ordinarily get no arm at all — see the V63-V69 note below — but the TOP @@ -899,6 +900,22 @@ the connect-time gate refuses a store that is perfectly current. Nothing in the that V108 through V111 all restated. The table is named only in the probe line above and NOT in this prose, per the V71 finding: the coverage ratchet strips information_schema lines but cannot strip a comment. */ + /* V113 (#2138 phase 1): config.config_monitored_servers gains the per-server remediation + credential and collect.plan_force_actions gains the journal's actor. COLUMN-existence sentinel on + the actor column: both objects the rung touches already exist (the registry since V17, the + journal since V107), so table existence cannot separate the rungs, and the actor is the one the + own-forces-only invariant turns on - a store without it cannot tell an operator's force from the + bot's, which is the property the rung exists to establish. Deliberately not a credential column: + those are the secret and non-secret halves of one optional feature, and a probe line naming one + reads as though the viewer needed to see it. The TOP rung now, so it must map EXACTLY or the + connect-time gate refuses a store that is perfectly current. Nothing in the viewer reads the + actor column yet - the own-forces-only filter is service-side - so this arm is the + don't-under-report guard, the V44/V53 reasoning that V108 through V112 all restated. */ + if (hasRemediationCredentialAndActor) + { + return 113; + } + if (hasCollectorStallProbes) { return 112; diff --git a/Darling/README.md b/Darling/README.md index 106114c255..0b18859430 100644 --- a/Darling/README.md +++ b/Darling/README.md @@ -410,6 +410,12 @@ Two mutually exclusive modes — setting both `managed: true` and `connectionStr | `encryptMode` | `"Mandatory"` | `Mandatory` / `Strict` / `Optional`; unknown values fail closed to `Mandatory` | | `multiSubnetFailover` | `false` | | | `excludedDatabases` | `[]` | Databases excluded from collection | +| `remediationUsername` | *(none)* | **Optional second credential, for operator-initiated remediation only (#2138).** The credential above stays read-only forever and is never used for a write. Omit both remediation keys — the default — and this server has no remediation surface at all: there is nothing to enable and nothing to disable. Both halves are required; one alone counts as unarmed and the service says so at connect. Always SQL auth when set, so there is no `remediationAuth` key: an integrated remediation identity would be the service account, which is the monitoring identity | +| `remediationEncryptedPassword` | *(none)* | The remediation credential's secret: same DPAPI blob from `--encrypt-password`, or an `env:NAME` / `file:/path` reference. There is deliberately **no plaintext variant** of this one — a wrong monitoring password fails a read, a wrong remediation password fails a write against a production server | + +**Least privilege for the remediation credential.** Grant `ALTER` on each database you intend to force a plan in — that is what `sp_query_store_force_plan` / `sp_query_store_unforce_plan` need. `ALTER SERVER STATE` is needed *only* for the evict-first lever, because that is what a targeted `DBCC FREEPROCCACHE(plan_handle)` requires, and it is the expensive grant. It is genuinely optional: without it the flow degrades to force-only **with a named reason**, never silently. The service does not guess per platform — it asks the server with a read-only `has_perms_by_name` check, which answers for the credential's effective permissions however the grant arrived. Azure SQL Database has no `DBCC FREEPROCCACHE` at all, which no grant changes, and that is reported as its own reason rather than as a permission problem. + +> Phase 1 ships **no consumer of this credential**. It is accepted, stored and resolvable so the arming model can be reviewed and provisioned ahead of the write path; the service logs at connect that an armed server is inert in this build. Nothing in this release can write to a monitored server. ### capturePlans (boolean, optional) diff --git a/Darling/tools/provision-roles.sql b/Darling/tools/provision-roles.sql index eaf7eb33e6..ea82164154 100644 --- a/Darling/tools/provision-roles.sql +++ b/Darling/tools/provision-roles.sql @@ -104,7 +104,10 @@ GRANT SELECT (server_id, name, host, database, auth, username, encrypt_mode, tru -- V68: engine + port. Non-secret, exactly like host. engine, port, -- V107 (#2138): the force-plan bot's per-server arm state. Non-secret, exactly like is_enabled. - plan_force_bot_enabled) + plan_force_bot_enabled, + -- V113 (#2138 phase 1): the remediation credential's login name. Non-secret, exactly like + -- username; remediation_encrypted_password is deliberately NOT granted. + remediation_username) ON config.config_monitored_servers TO viewer; REVOKE SELECT ON config.config_command FROM viewer; GRANT SELECT (command_id, created_at, requested_by, command_type, target_server_id, status, claimed_at, diff --git a/Lite.Tests/OperatorRemediationLiteDivergencePinTests.cs b/Lite.Tests/OperatorRemediationLiteDivergencePinTests.cs new file mode 100644 index 0000000000..89daa6d958 --- /dev/null +++ b/Lite.Tests/OperatorRemediationLiteDivergencePinTests.cs @@ -0,0 +1,260 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.IO; +using System.Linq; +using PerformanceMonitor.Analysis; +using Xunit; + +namespace Lite.Tests; + +/// +/// #2138 phase 1 lands in Darling only, and this pin exists so that stays a DECISION rather than +/// something the next lane inherits. +/// +/// Why it is not in Lite, and no longer provisionally. Settled on #2138: remediation +/// execution is Darling-only, permanently. The rule was already stated in Lite's own code, but with two +/// different reasons, and only one of them survived — which is exactly the kind of thing this pin exists +/// to keep honest. The obsolete half named the deprecated Dashboard SKU as the owner, so it pointed at +/// nothing live. The half that decides it is Lite's STORE: a DuckDB file is per-workstation, so a +/// remediation journal kept there cannot be the shared audit trail such a journal exists to be — two +/// operators acting on one server would each hold half of it and neither could see the other's forces — +/// and it cannot carry the bot's own-forces-only invariant, which is a predicate plus a self-reference +/// within ONE shared store. No deprecation can invalidate that, which is why the wording now rests on it. +/// +/// The shared pure seam ( / ) +/// still lives in PerformanceMonitor.Analysis, which Lite already references — so the divergence is +/// about which SKU may ACT, never about duplicated logic. +/// +/// What this pin does. It asserts the divergence is COMPLETE and its stated reason still +/// exists. Both halves matter, and they fail in opposite directions: if Lite gains a remediation +/// credential or journal without the scope decision being revisited, the first assertions fail; if the +/// scope statements this divergence rests on are deleted or reworded away, the last one fails, and this +/// file's whole justification has to be rewritten rather than quietly outliving its premise. +/// +/// It is deliberately NOT a claim that Lite should never act. It is a claim that today it does not, +/// on a stated basis, and that changing either fact is a visible edit here. +/// +public sealed class OperatorRemediationLiteDivergencePinTests +{ + /// + /// The shared seam is reachable from Lite (it is in an assembly Lite references) but unused by it — + /// so the divergence is "Lite does not act", not "Lite could not". Asserted by resolving the types + /// from the running Lite test assembly's reference graph, which is a stronger statement than a source + /// grep: it proves the reference really exists rather than that a string appears. + /// + [Fact] + public void TheSharedSeamIsReachableFromLite_SoNeitherSkuWouldNeedItsOwnCopy() + { + Assert.NotNull(typeof(OperatorRemediationGate)); + Assert.NotNull(typeof(OperatorRemediationFlow)); + + /* And it is the SAME assembly the Darling side consults — one policy path, per the #2146 + contract. If these ever differ, "the bot executes exactly the verdict object agents inspect" + has stopped being true at the assembly level. */ + Assert.Equal( + typeof(FactRemediation).Assembly.GetName().Name, + typeof(OperatorRemediationGate).Assembly.GetName().Name); + } + + /// + /// The tokens that can only appear under Lite/ if Lite gained the phase-1 capability — a + /// credential slot, the shared gate being USED rather than merely referenceable, the executor seam, + /// the journal, or a force/evict statement. + /// + /// Not a scan for the word "remediation". That was the first version of this test and it + /// found 12 files, every one of them correct: Lite renders remediation_action_json as + /// copy-paste advice, which is a first-class Lite feature and the very thing the advise-only stance + /// describes. A scan that fires on the feature working is one that gets an exclusion list bolted onto + /// it until it means nothing. These tokens name the CAPABILITY instead, so the scan and the stance + /// agree: rendering advice is expected, executing it is the thing that is absent. + /// + [Fact] + public void LiteHasNoRemediationCredential_NoExecutorSeam_AndNoJournal() + { + var capabilityTokens = new[] + { + /* the credential */ + "RemediationUsername", "RemediationEncryptedPassword", + "remediation_username", "remediation_encrypted_password", + /* the shared seam, used rather than referenceable */ + nameof(OperatorRemediationGate), nameof(OperatorRemediationFlow), + /* the write seam, the journal, and the policy */ + "IPlanForceExecutor", "PlanForceActionRecord", "plan_force_actions", "ForcePlanBotPolicy", + /* the statements themselves */ + "sp_query_store_force_plan", "sp_query_store_unforce_plan", "FREEPROCCACHE", + }; + + var files = LiteSourceFiles(); + var offenders = + (from path in files + let text = File.ReadAllText(path) + from token in capabilityTokens + where text.Contains(token, StringComparison.Ordinal) + select $"{RelativeToRepo(path)} ({token})").ToList(); + + Assert.True( + offenders.Count == 0, + "Lite gained a #2138 phase-1 capability. Phase 1 is Darling-only because Lite states it has " + + "no privileged remediation path (see this class's remarks); if that scope decision has been " + + "revisited, delete this pin in the same change rather than adding an exclusion:\n " + + string.Join("\n ", offenders)); + + /* Two controls, because the assertion above is an absence and an absence has two ways to be + vacuous. First: the enumeration really read Lite — Lite is a 260-odd-file app, so a floor well + below that catches a scan that returned nothing. */ + Assert.True(files.Count >= 100, $"the Lite source enumeration returned {files.Count} files"); + + /* Second: the scan can actually fire. Run the same token list against a Darling file that DOES + carry the capability — if this finds nothing, the matcher is broken and the absence above + proves nothing about Lite. */ + var darlingJournal = ParitySource.ReadFile( + "Darling/PerformanceMonitor.Darling.Service/PgPlanForceActionStore.cs"); + Assert.Contains( + capabilityTokens, + token => darlingJournal.Contains(token, StringComparison.Ordinal)); + } + + /// + /// Lite's DuckDB schema carries no plan-force journal TABLE. This is the half that matters most: a + /// journal added to Lite while Lite cannot act would be a permanently-empty table, which reads to + /// every later consumer as "the feature is here and nothing has happened" rather than "the feature is + /// not here". + /// + /// Asserted on table NAMES, not on substrings of the DDL — and this is the second time + /// this file's first instinct was a scan too broad to survive its own subject. The DDL genuinely + /// contains plan_force: plan_correction carries a generated + /// last_good_plan_force_failure_reason column, which is the MONITORED SERVER'S own Query Store + /// forcing-failure reason — the opposite of a bot's audit trail of its own writes, and a column Lite + /// has read for a long time. A statement-substring scan reported that as a journal, which is a scan + /// that has to be relaxed to ship. The claim is about a table existing, so the test asks about + /// tables. + /// + [Fact] + public void LitesSchemaHasNoPlanForceJournalTable() + { + var tableNames = PerformanceMonitorLite.Database.Schema.GetAllTableStatements() + .Select(TableNameOf) + .Where(name => name.Length > 0) + .ToList(); + + /* Two positive controls, because the assertion below is an absence. The enumeration produced a + real schema... */ + Assert.Contains("servers", tableNames); + + /* ...and the NAME EXTRACTION works, rather than silently returning empty strings that would make + every absence assertion vacuous. plan_correction is the table whose COLUMN caused this test's + first version to fail, so its presence here is also the proof that the new form discriminates + the column from a table. */ + Assert.Contains("plan_correction", tableNames); + Assert.True(tableNames.Count >= 40, $"only {tableNames.Count} table names parsed out of the schema"); + + var journals = tableNames + .Where(name => + name.Contains("plan_force", StringComparison.OrdinalIgnoreCase) || + name.Contains("remediation", StringComparison.OrdinalIgnoreCase)) + .ToList(); + + Assert.True( + journals.Count == 0, + "Lite's schema gained a plan-force/remediation journal table: " + string.Join(", ", journals)); + } + + /// + /// The table name out of a CREATE TABLE [IF NOT EXISTS] name (…), or empty when the statement + /// is not one. Empty rather than throwing so a future non-CREATE statement in the list does not fail + /// this test for an unrelated reason — the count control above is what stops an all-empty parse + /// reading as agreement. + /// + private static string TableNameOf(string statement) + { + var match = System.Text.RegularExpressions.Regex.Match( + statement, + @"CREATE\s+TABLE\s+(?:IF\s+NOT\s+EXISTS\s+)?(?[A-Za-z0-9_]+)", + System.Text.RegularExpressions.RegexOptions.IgnoreCase + | System.Text.RegularExpressions.RegexOptions.CultureInvariant); + + return match.Success ? match.Groups["name"].Value : string.Empty; + } + + /// + /// The stated basis for the divergence still exists in Lite's own source. Without this, the pin above + /// could outlive its reason: someone could delete the advise-only remarks, and the divergence would + /// carry on being enforced by a test whose justification had evaporated. Matched on the load-bearing + /// phrases rather than whole paragraphs so a rewording that keeps the decision keeps the pin. + /// + /// The RULE and the REASON are asserted separately, because the wording fix on #2138 is the + /// case that showed why: the rule never changed and one of its two reasons died, so a test that only + /// checked the rule would have gone on passing over a justification that named a deprecated SKU as + /// the owner of a capability. Asserting the reason too is what made this pin redden and get updated + /// rather than quietly outlive its premise. + /// + [Fact] + public void TheAdviseOnlyStatementsAndTheirDurableReason_StillExist() + { + var item = ParitySource.ReadFile("Lite/Analysis/Recommendations/LiteRecommendationItem.cs"); + var card = ParitySource.ReadFile("Lite/Analysis/Recommendations/LiteRecommendationsViewModel.cs"); + var tab = ParitySource.ReadFile("Lite/Controls/RecommendationsTab.xaml.cs"); + + /* The rule. Unchanged by the #2138 wording fix, and still the thing an operator relies on. */ + Assert.Contains("NO in-app Apply/execute path", item, StringComparison.Ordinal); + Assert.Contains("Lite is ADVISE-ONLY", card, StringComparison.Ordinal); + Assert.Contains("NO privileged remediation execution", card, StringComparison.Ordinal); + Assert.Contains("NO Apply button", tab, StringComparison.Ordinal); + + /* The reason, in all three places that state the rule — the store argument, which a deprecation + cannot reach. Matched on "per-workstation" because that is the load-bearing word: it is what + makes a journal there unable to be a shared audit trail. */ + foreach (var (file, text) in new[] + { + ("LiteRecommendationItem.cs", item), + ("LiteRecommendationsViewModel.cs", card), + ("RecommendationsTab.xaml.cs", tab), + }) + { + Assert.True( + text.Contains("per-workstation", StringComparison.Ordinal), + $"{file} states the advise-only rule without the store reason it now rests on. If the " + + "reason has moved, move this assertion with it — do not drop it, or the rule outlives " + + "its justification again (#2138)."); + } + + /* And the DEAD reason is gone from every one of them: it named the deprecated Dashboard SKU as + the owner of remediation execution, which is a claim about a live division of responsibility + that no longer exists. Reinstating it would be a regression to a justification that points + nowhere. */ + foreach (var (file, text) in new[] + { + ("LiteRecommendationItem.cs", item), + ("LiteRecommendationsViewModel.cs", card), + ("RecommendationsTab.xaml.cs", tab), + }) + { + Assert.DoesNotContain("Dashboard-only", text, StringComparison.Ordinal); + Assert.DoesNotContain("per project scope", text, StringComparison.Ordinal); + } + + /* Positive control for those two absences: the same read DOES find the Dashboard named in these + files as a UI COMPARISON, which is deliberately left alone — its reason is the surface's tab + shape, not a scope claim, so the deprecation does not touch it. Without this control the + assertions above would pass just as well against a failed read. */ + Assert.Contains( + "the Dashboard) no \"Open in Active Queries\" deep-link", card, StringComparison.Ordinal); + Assert.Contains("Dashboard's \"Open in Active Queries\" deep-link is omitted", tab, StringComparison.Ordinal); + } + + private static System.Collections.Generic.List LiteSourceFiles() => + Directory.EnumerateFiles(Path.Combine(ParitySource.RepoRoot(), "Lite"), "*.cs", SearchOption.AllDirectories) + .Where(p => !p.Contains($"{Path.DirectorySeparatorChar}obj{Path.DirectorySeparatorChar}", StringComparison.Ordinal)) + .Where(p => !p.Contains($"{Path.DirectorySeparatorChar}bin{Path.DirectorySeparatorChar}", StringComparison.Ordinal)) + .ToList(); + + private static string RelativeToRepo(string path) => + Path.GetRelativePath(ParitySource.RepoRoot(), path).Replace('\\', '/'); +} diff --git a/Lite/Analysis/Recommendations/LiteRecommendationItem.cs b/Lite/Analysis/Recommendations/LiteRecommendationItem.cs index 9c4a4a8c72..3c6a13d29d 100644 --- a/Lite/Analysis/Recommendations/LiteRecommendationItem.cs +++ b/Lite/Analysis/Recommendations/LiteRecommendationItem.cs @@ -32,8 +32,13 @@ public enum LiteRecommendationSeverity /// . /// /// -/// Lite produces a COPYABLE remediation command but has NO in-app Apply/execute path (SQL-side -/// remediation execution is Dashboard-only, per project scope). A Lite card offers the diagnosis, the +/// Lite produces a COPYABLE remediation command but has NO in-app Apply/execute path, permanently +/// (#2138). The reason is Lite's STORE, not a division of labour between SKUs: Lite's DuckDB file is +/// per-workstation, and a remediation journal kept there could not be the audit trail such a journal +/// exists to be — two operators acting on the same server would each hold half of it and neither could +/// see the other's forces. It also could not carry the auto-force bot's own-forces-only invariant, which +/// is a predicate plus a self-reference WITHIN one shared store. So the execute path belongs to the SKU +/// with a shared store, and no deprecation can move it back. A Lite card offers the diagnosis, the /// copy-paste T-SQL rendered from the finding's persisted /// (the SAME shared renderer the Darling /// viewer uses, so the commands are byte-identical), and an "Ask AI" MCP prompt — the operator runs the diff --git a/Lite/Analysis/Recommendations/LiteRecommendationsViewModel.cs b/Lite/Analysis/Recommendations/LiteRecommendationsViewModel.cs index be5e24c819..bc39bdfbe3 100644 --- a/Lite/Analysis/Recommendations/LiteRecommendationsViewModel.cs +++ b/Lite/Analysis/Recommendations/LiteRecommendationsViewModel.cs @@ -42,7 +42,9 @@ public enum LiteRecommendationsState /// binds to, so the affordance model and the Ask-AI prompt are unit-testable. /// /// -/// Lite is ADVISE-ONLY. There is NO Apply button, NO privileged remediation execution, and (unlike +/// Lite is ADVISE-ONLY, and permanently so (#2138): its store is a per-workstation DuckDB file, which +/// cannot hold the shared audit trail of writes-to-a-server that a remediation journal has to be. There +/// is NO Apply button, NO privileged remediation execution, and (unlike /// the Dashboard) no "Open in Active Queries" deep-link — the Lite Recommendations surface is a /// shared tab with a server selector, not a per-server tab that owns an Active Queries view, so that /// affordance is omitted gracefully. Every card offers "Ask AI" (copies an MCP investigation diff --git a/Lite/Controls/RecommendationsTab.xaml.cs b/Lite/Controls/RecommendationsTab.xaml.cs index a359cba4bf..184068a4a2 100644 --- a/Lite/Controls/RecommendationsTab.xaml.cs +++ b/Lite/Controls/RecommendationsTab.xaml.cs @@ -27,8 +27,9 @@ namespace PerformanceMonitorLite.Controls; /// Dashboard's Recommendations tab UX, but ADVISE-ONLY. /// /// -/// Lite is local-DuckDB and SQL-side remediation is Dashboard-only (per project scope), so there -/// is NO Apply button and NO privileged execution path here. Each card offers "Ask AI" (copies an +/// Lite is local-DuckDB, so there is NO Apply button and NO privileged execution path here, and there +/// permanently will not be (#2138): a per-workstation store cannot hold a shared audit trail of writes +/// to a server, which is what a remediation journal has to be. Each card offers "Ask AI" (copies an /// MCP investigation prompt); cards whose finding carries a derivable copy-paste statement also /// offer "Copy fix". The Dashboard's "Open in Active Queries" deep-link is omitted: the Lite /// surface is a shared tab with a server selector, not a per-server tab that owns an Active Queries diff --git a/PerformanceMonitor.Analysis/ForcePlanBotPolicy.cs b/PerformanceMonitor.Analysis/ForcePlanBotPolicy.cs index 591c698a4f..4186c54a85 100644 --- a/PerformanceMonitor.Analysis/ForcePlanBotPolicy.cs +++ b/PerformanceMonitor.Analysis/ForcePlanBotPolicy.cs @@ -69,9 +69,29 @@ public sealed record ForcePlanBotSettings public int FinalReviewMinutes { get; init; } = 1440; /// Executions the forced query must accumulate before a checkpoint judges cost — the - /// same floor detection uses, so the review never rules on thinner evidence than the decision did. + /// same floor detection uses, so the review never rules on thinner evidence than the decision did. + /// Also the executions limb of the operator flow's post-eviction observation window + /// (), which takes it as a parameter rather than + /// restating the number: one floor, so an evict-then-observe verdict and the self-review that later + /// judges the same query cannot rule on different amounts of evidence. public int MinReviewExecutions { get; init; } = 25; + /// + /// The elapsed limb of the post-eviction observation window, in minutes — the operator flow observes + /// until whichever comes first of executions or this. + /// + /// A TIMEOUT rather than a second measurement, and the state machine treats it as one: a window + /// this limb closed cannot support a cost verdict, because it says the time is up and nothing about + /// how much ran inside it (). Its job is to stop an observation on + /// a query nobody called from waiting forever. + /// + /// Lives on the bot's settings rather than beside the operator flow because the design has the + /// bot reuse this exact sequence in phase 2. One window, one knob — a separate operator-side default + /// would let a human and the bot observe the same eviction for different lengths of time and reach + /// different verdicts about it. + /// + public int ObservationWindowMinutes { get; init; } = 30; + /// /// The net-benefit bar: post-force cpu/exec must be at or below this fraction of the regressed /// baseline (default 0.75 = at least 25% better) or the self-review unforces. "No worse" is @@ -97,6 +117,10 @@ public ForcePlanBotSettings Normalize() => this with FirstReviewMinutes = Math.Clamp(FirstReviewMinutes, 5, 1440), FinalReviewMinutes = Math.Clamp(FinalReviewMinutes, Math.Clamp(FirstReviewMinutes, 5, 1440), 10080), MinReviewExecutions = Math.Clamp(MinReviewExecutions, 1, 100000), + /* Floor of 1 minute: a zero or negative window would close the observation on the same pass the + eviction ran, so every eviction would be judged before the optimizer had compiled anything — + the elapsed limb's whole job is to be a bound, and an instant bound is not one. */ + ObservationWindowMinutes = Math.Clamp(ObservationWindowMinutes, 1, 1440), NetBenefitRatio = double.IsFinite(NetBenefitRatio) ? Math.Clamp(NetBenefitRatio, 0.05, 1.0) : 0.75, }; } diff --git a/PerformanceMonitor.Analysis/OperatorRemediationFlow.cs b/PerformanceMonitor.Analysis/OperatorRemediationFlow.cs new file mode 100644 index 0000000000..ab28c2b0d5 --- /dev/null +++ b/PerformanceMonitor.Analysis/OperatorRemediationFlow.cs @@ -0,0 +1,300 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; + +namespace PerformanceMonitor.Analysis; + +/// +/// What live Query Store showed for one target after a targeted eviction — the inputs the re-score +/// judges. Every field is measured on the server AFTER the evict, against the pre-evict evidence the +/// journal already recorded. +/// +/// Executions accumulated since the eviction, across all plans for the +/// query. Zero is a real and common answer: an evicted plan for a query nobody called back is not +/// evidence of anything. +/// Wall-clock since the eviction. +/// The query_plan_hash executions have been attributed to SINCE the +/// eviction — read from post-eviction runtime-stats intervals, not from whether a plan row exists. Null +/// when no post-eviction interval has attributed one yet, which is the normal reading for the first +/// several minutes. +/// +/// The distinction is load-bearing, not pedantic. A targeted +/// DBCC FREEPROCCACHE(plan_handle) evicts from the PLAN CACHE; Query Store keeps its plan row — +/// that is the whole difference between the two stores. So "is the regressed plan's hash present in Query +/// Store" is always yes after an eviction and is evidence of nothing. Only "which plan did post-eviction +/// executions run under" answers the question evict-first asks, and that fact does not exist until +/// runtime stats for those executions have been flushed and attributed. +/// +/// Compared against the regressed plan's hash, NOT the best plan's: the question is "did the +/// optimizer make the same mistake again", and only the regressed hash answers that one. +/// Post-evict cpu/exec in microseconds, or null when the server has +/// nothing to average yet. +public sealed record RemediationObservation( + long ObservedExecutions, + TimeSpan ElapsedSinceEvict, + string? ActivePlanHash, + double? ObservedCpuPerExecUs); + +/// +/// Which limb of the observation window closed it. Both limbs are real answers and they are NOT +/// interchangeable — see for why the executions limb can +/// support a cost verdict and the elapsed limb cannot. +/// +public enum ObservationWindowLimb +{ + /// Neither limb has fired; keep observing. + Open, + + /// Enough executions accumulated to judge cost — the evidence floor detection itself used. + Executions, + + /// The time limit expired. A TIMEOUT, not a measurement: it says the window is over, and + /// nothing at all about how much evidence arrived inside it. + Elapsed, +} + +/// +/// The verdict after the observation window closes — the four outcomes the design's step 2 names, plus the +/// honest fifth for a window that closed without enough evidence to rule. +/// +public enum RemediationObservationVerdict +{ + /// The window is still open. Not journaled; not a decision. + StillObserving, + + /// A different plan is active AND it is measurably cheaper. Done — nothing more to do, and + /// the force is NOT offered. + OptimizerRecovered, + + /// The optimizer compiled the same regressed plan again. The force becomes available as a + /// SECOND, separate operator decision; this verdict does not place it. + RegressedPlanReturned, + + /// Measurably worse than the regressed baseline the eviction was meant to escape. Journal and + /// stop — offering a force here would be acting against the only evidence we have. + Worse, + + /// The window closed without evidence that discriminates any of the above. Journaled as its + /// own outcome and the flow stops: the operator may start a fresh observation. Deliberately NOT folded + /// into , which would offer a force on the strength of not having + /// looked. + Inconclusive, +} + +/// One observation's whole answer, so a caller cannot take the verdict and drop the limb or the +/// reason (the record-return discipline the alert gates and use). +/// The decision. +/// Which limb closed the window ( while it is +/// still running). +/// Whether the operator's SECOND click becomes available. True only for +/// — a property of the value rather than +/// something each call site re-derives from the verdict, so no surface can offer a force after a verdict +/// that did not authorize one. +public sealed record RemediationObservationResult( + RemediationObservationVerdict Verdict, + ObservationWindowLimb Limb, + bool ForceOffered); + +/// +/// The evict-first observation state machine (#2138 phase 1, design step 2). Pure and static — no clock, +/// no I/O, no store; the caller measures and passes the numbers in, exactly like +/// , so the whole decision table is unit-testable without a host, a store +/// or a server. +/// +/// Human-armed by construction. Nothing here executes anything. The eviction happened before +/// the caller could ask this question, and the force — when this returns +/// — is a separate operator decision +/// that a separate call has to carry out. is +/// permission to DRAW a control, never permission to act. +/// +public static class OperatorRemediationFlow +{ + /// + /// The journaled decision strings for each verdict. A consumer API like + /// 's reasons — the audit trail is read by people and by agents, so + /// these are stable names, never re-spelled. + /// + public const string DecisionOptimizerRecovered = "optimizer_recovered"; + + public const string DecisionRegressedPlanReturned = "regressed_plan_returned"; + + public const string DecisionWorseAfterEvict = "worse_after_evict"; + + public const string DecisionObservationInconclusive = "observation_inconclusive"; + + /// + /// How much better than the pre-evict baseline counts as recovery, and how much worse counts as worse. + /// A dead band on purpose: cpu/exec on a live server moves a few percent for reasons that have nothing + /// to do with which plan compiled, and a state machine with no dead band would call that noise a + /// verdict. 25% mirrors 's bar so the flow and the + /// self-review that judges its forces do not disagree about what "better" means. + /// + public const double MaterialChangeRatio = 0.75; + + /// + /// Judge one observation. + /// + /// What the server showed after the eviction. + /// The query_plan_hash of the plan the eviction removed — + /// StructuredForcePlanTarget.LatestPlanHash, the verdict object's own field. + /// The pre-evict regressed cpu/exec the decision was taken on — + /// the verdict object's StructuredForcePlanEvidence.LatestCpuPerExecUs. Comparing against the + /// evidence the operator was SHOWN, rather than re-reading a baseline now, is what makes the outcome + /// auditable: the journal row holds both numbers and the comparison can be re-done by hand. + /// The executions limb. + /// Callers pass — there is no literal here, so + /// the floor cannot drift away from the one the review and detection use. + /// The elapsed limb. Callers pass + /// as a . + public static RemediationObservationResult Observe( + RemediationObservation observation, + string? regressedPlanHash, + double baselineCpuPerExecUs, + int minObservationExecutions, + TimeSpan observationWindow) + { + if (observation is null) + { + throw new ArgumentNullException(nameof(observation)); + } + + var limb = Limb(observation, minObservationExecutions, observationWindow); + if (limb == ObservationWindowLimb.Open) + { + return new RemediationObservationResult( + RemediationObservationVerdict.StillObserving, limb, ForceOffered: false); + } + + /* Plan IDENTITY first, and it carries a LOWER evidence requirement than cost — but not a zero + one, and the difference between those two readings is why this sits below the window guard + rather than above it. + + Lower: which plan the optimizer chose is not a statistical quantity, so a handful of attributed + post-eviction executions settle it, and this arm is legitimate on a window the timeout closed + with three. Cost is statistical and gets the executions floor below. Collapsing the two would + either refuse to report a plan that demonstrably came back, or claim a cost improvement measured + on nothing. + + Not zero, which is the part worth stating because the code reads as though it could run on every + call: the fact this arm tests does not EXIST before some executions have been attributed. + FREEPROCCACHE evicts the plan cache and Query Store keeps its plan row, so the regressed hash is + present the instant after the eviction and stays present — see ActivePlanHash's remarks. Running + this arm while the window is still open would therefore not report an early recompile; it would + report the pre-eviction plan, on every first call, and offer a force on it. Query Store's own + flush interval (DATA_FLUSH_INTERVAL_SECONDS, 900 by default) is why the elapsed limb's 30 + minutes is the right order of magnitude rather than a round number. + + There is a second reason to keep both verdicts behind one window even if the instrument were + instantaneous: OptimizerRecovered needs the executions floor, so an identity arm that fired + earlier would make the FORCE the quick answer and "nothing here needs pinning" the slow one. + That is the wrong asymmetry for a lever whose premise is that the cheapest fix pins nothing. + Pinned by OperatorRemediationFlowTests' window-open-with-a-matching-hash cases. */ + if (SameHash(observation.ActivePlanHash, regressedPlanHash)) + { + return new RemediationObservationResult( + RemediationObservationVerdict.RegressedPlanReturned, limb, ForceOffered: true); + } + + /* Below the cost floor nothing about cost can be claimed. Reached by the elapsed limb almost by + definition, and reachable by the executions limb only when the server gave us no average — both + are "we did not learn anything", which is a result and gets journaled as one. */ + if (limb == ObservationWindowLimb.Elapsed && observation.ObservedExecutions < minObservationExecutions) + { + return Inconclusive(limb); + } + + if (observation.ObservedCpuPerExecUs is not double observed || !double.IsFinite(observed) || + baselineCpuPerExecUs <= 0 || !double.IsFinite(baselineCpuPerExecUs)) + { + return Inconclusive(limb); + } + + if (observed <= baselineCpuPerExecUs * MaterialChangeRatio) + { + /* A cheaper plan the optimizer found on its own. The force is deliberately NOT offered: the + whole point of evict-first is that the cheapest fix is the one that pins nothing. */ + return new RemediationObservationResult( + RemediationObservationVerdict.OptimizerRecovered, limb, ForceOffered: false); + } + + if (observed >= baselineCpuPerExecUs / MaterialChangeRatio) + { + /* Worse than what the operator was already unhappy with. Stop — and specifically do not offer + the force, because the plan now running is not the regressed plan we have a known-better + alternative to, so there is nothing here the force is the answer to. */ + return new RemediationObservationResult( + RemediationObservationVerdict.Worse, limb, ForceOffered: false); + } + + /* Inside the dead band: a different plan, indistinguishable in cost. Not recovery (nothing got + better), not worse, and not the regressed plan returning. */ + return Inconclusive(limb); + } + + /// + /// Which limb closed the window, if either. Executions is checked first so a window that satisfied + /// BOTH limbs reports the one that carries evidence — the elapsed limb would be true of the same + /// observation and would suppress a cost verdict the executions actually support. + /// + public static ObservationWindowLimb Limb( + RemediationObservation observation, + int minObservationExecutions, + TimeSpan observationWindow) + { + if (observation is null) + { + throw new ArgumentNullException(nameof(observation)); + } + + if (observation.ObservedExecutions >= minObservationExecutions) + { + return ObservationWindowLimb.Executions; + } + + return observation.ElapsedSinceEvict >= observationWindow + ? ObservationWindowLimb.Elapsed + : ObservationWindowLimb.Open; + } + + /// The journal's decision string for a verdict. Throws on + /// rather than inventing a string: an + /// in-progress observation is not a decision, and a caller journaling one has a bug this hides. + public static string DecisionFor(RemediationObservationVerdict verdict) => verdict switch + { + RemediationObservationVerdict.OptimizerRecovered => DecisionOptimizerRecovered, + RemediationObservationVerdict.RegressedPlanReturned => DecisionRegressedPlanReturned, + RemediationObservationVerdict.Worse => DecisionWorseAfterEvict, + RemediationObservationVerdict.Inconclusive => DecisionObservationInconclusive, + _ => throw new ArgumentOutOfRangeException( + nameof(verdict), verdict, "an observation still running is not a journalable decision"), + }; + + private static RemediationObservationResult Inconclusive(ObservationWindowLimb limb) => + new(RemediationObservationVerdict.Inconclusive, limb, ForceOffered: false); + + /* Query Store renders a plan hash as 0x-prefixed hex, and the two sides of this comparison arrive + from different places (the persisted target, and a live read), so case and prefix are not + guaranteed to match even when the hashes do. Compared as normalized text rather than parsed to + bytes because a malformed hash must make this return false — not throw inside a verdict. */ + private static bool SameHash(string? left, string? right) + { + var a = Normalize(left); + var b = Normalize(right); + + /* An absent hash on either side never matches. A null ActivePlanHash means the server has not + attributed a post-evict plan, which is the opposite of evidence that the regressed one is back. */ + return a.Length > 0 && b.Length > 0 && string.Equals(a, b, StringComparison.OrdinalIgnoreCase); + } + + private static string Normalize(string? hash) + { + var text = (hash ?? string.Empty).Trim(); + return text.StartsWith("0x", StringComparison.OrdinalIgnoreCase) ? text.Substring(2) : text; + } +} diff --git a/PerformanceMonitor.Analysis/OperatorRemediationGate.cs b/PerformanceMonitor.Analysis/OperatorRemediationGate.cs new file mode 100644 index 0000000000..da8d2e23e1 --- /dev/null +++ b/PerformanceMonitor.Analysis/OperatorRemediationGate.cs @@ -0,0 +1,189 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; + +namespace PerformanceMonitor.Analysis; + +/// +/// Whether a server's remediation credential can drive the targeted plan-cache eviction, and the NAMED +/// reason when it cannot (#2138 phase 1). +/// +/// Two independent facts have to hold, and they fail for different reasons an operator would fix +/// differently, so they are separate members rather than one boolean. The engine either has the statement +/// or does not (nothing an operator can grant changes that); the credential either holds +/// ALTER SERVER STATE or does not (a grant fixes that one). Collapsing them would tell an operator +/// to ask for a permission that does not exist on their platform. +/// +/// Neither fact is predicted from a per-platform table. is +/// answered by the engine edition the registration upsert already stamps, and +/// by a read-only has_perms_by_name probe run AS the +/// remediation credential. A table of "platforms that allow plan-handle FREEPROCCACHE" would be a claim +/// about a managed service's grant set, which is the vendor's to change without telling us, and it would +/// go stale in the direction that keeps passing. +/// +/// The engine edition has DBCC FREEPROCCACHE at all. +/// The probe said this credential may run it. Null means the +/// probe has not run yet — deliberately distinct from false, because "not asked" and "asked and denied" +/// send an operator to different places. +public sealed record EvictCapability(bool StatementSupported, bool? CredentialHoldsAlterServerState) +{ + /// Nothing known yet — the shape before any probe has run. + public static EvictCapability Unprobed { get; } = new(true, null); +} + +/// +/// The named reasons evict-first is unavailable. Consumer-API strings like the alert fact names and +/// 's reasons: add new ones freely, never redefine an existing one. +/// +public static class EvictDegradeReasons +{ + /// DBCC FREEPROCCACHE is not a statement this engine edition has (Azure SQL Database). + /// Not a permission problem and no grant fixes it. + public const string UnsupportedOnPlatform = "evict_unsupported_on_platform"; + + /// The remediation credential lacks ALTER SERVER STATE. A grant fixes this one, which is + /// why it is not the same reason as . + public const string PermissionDenied = "evict_permission_denied"; + + /// The capability probe has not run yet, so evict-first is not offered — refusing to guess + /// rather than attempting a write to find out. + public const string CapabilityUnknown = "evict_capability_unknown"; +} + +/// +/// What an operator may do to ONE force-plan target on ONE server. Deliberately a value that is +/// absent (null from ) rather than a value carrying +/// a disabled flag — see that method's remarks. +/// +/// The evict-then-observe path is available. When false, +/// names why and the force is offered on its own. +/// One of , or null when evict-first +/// IS offered. Never null-with-EvictFirstOffered-false: the degrade always carries its reason, which is +/// what stops it happening silently. +public sealed record OperatorRemediationSurface(bool EvictFirstOffered, string? EvictUnavailableReason); + +/// +/// The #2138 phase-1 arming gate: does an operator get an action surface for this force-plan target, and +/// if so, which levers. +/// +/// The gate EXECUTES the verdict object; it does not re-derive one. Eligibility comes from +/// and — +/// the fields FactRemediation.BuildStructuredRemediation fills from +/// FactRemediation.ForcePlanBlockers, which is the same output an MCP consumer reads. Nothing here +/// looks at ParameterSensitivityCoFired, ReplicaRole or any other evidence field to reach a +/// verdict of its own. If a future change makes this file ask an evidence question, that is the second +/// policy path the whole design exists to avoid. +/// +/// Why the return is nullable rather than an "enabled" flag. The design's requirement is that +/// a server with no remediation credential has NO phase-1 surface — not a greyed-out button with a tooltip. +/// A record carrying Enabled = false invites exactly that button, because a binding to a present +/// object is the easy thing to write. Absence is unrenderable: a view binding +/// Visibility to null-ness cannot accidentally draw a disabled control, and a view-model with a +/// null surface has nothing to bind a command to. The nullability is the mechanism, not a convention. +/// +/// Pure and static, no clock and no I/O, matching — both SKUs' view +/// models call this and neither can hold a different opinion. +/// +public static class OperatorRemediationGate +{ + /// + /// The surface for one target, or null for no surface at all. + /// + /// The verdict object, as an MCP consumer would read it. + /// This server has an opt-in remediation credential. + /// False for every server until an operator enters one; there is no default and no fallback to the + /// monitoring credential, which stays read-only forever. + /// What the eviction lever can do here — see . + public static OperatorRemediationSurface? SurfaceFor( + StructuredForcePlanTarget target, + bool remediationCredentialConfigured, + EvictCapability evict) + { + if (target is null) + { + return null; + } + + /* No credential, no surface. First check on purpose: an operator who has not armed this server + should not be able to tell an eligible target from an ineligible one through the presence of a + control, because that is how a surface starts existing "just to explain itself". */ + if (!remediationCredentialConfigured) + { + return null; + } + + /* Both halves of the verdict, and disagreement REFUSES. Eligible is defined as + blockers.Count == 0 where the projection is built, so on any object that projection produced the + two agree and the second read is free. It is not free on an object assembled anywhere else — + a hand-built or deserialized target with Eligible true and a blocker listed would arm on the + flag alone. Reading both means the only way to get a surface is for both to say so, and the + direction a mismatch fails in is "no surface", which is the safe one. */ + if (!target.Eligible) + { + return null; + } + + if (target.Blockers is { Count: > 0 }) + { + return null; + } + + var reason = EvictUnavailableReason(evict); + return reason is null + ? new OperatorRemediationSurface(EvictFirstOffered: true, EvictUnavailableReason: null) + : new OperatorRemediationSurface(EvictFirstOffered: false, EvictUnavailableReason: reason); + } + + /// + /// Why evict-first is unavailable, or null when it is available. Split out so the degrade path can be + /// exercised without an eligible target to hang it on. + /// + /// Precedence is platform-then-permission, and it matters: on an engine that has no + /// DBCC FREEPROCCACHE the permission question is meaningless, and reporting + /// there would send an operator to ask a cloud + /// provider for a grant that would change nothing. + /// + public static string? EvictUnavailableReason(EvictCapability evict) + { + if (evict is null) + { + return EvictDegradeReasons.CapabilityUnknown; + } + + if (!evict.StatementSupported) + { + return EvictDegradeReasons.UnsupportedOnPlatform; + } + + return evict.CredentialHoldsAlterServerState switch + { + null => EvictDegradeReasons.CapabilityUnknown, + false => EvictDegradeReasons.PermissionDenied, + true => null, + }; + } + + /// + /// The read-only probe behind , run AS the + /// remediation credential against the target server. + /// + /// has_perms_by_name(NULL, NULL, ...) is the server-scope form, and it answers for the + /// EFFECTIVE permissions of the login executing it — which is the only question that matters, because + /// the grant can arrive through a server role, through CONTROL SERVER, or directly, and an + /// operator on a managed platform generally cannot tell which they were given. Asking the server beats + /// enumerating the routes. + /// + /// It is a SELECT. Running it costs nothing, changes nothing, and is safe against a server whose + /// remediation credential turns out to be wrong — which is why the capability is probed rather than + /// discovered by attempting the eviction and reading the error. + /// + public const string AlterServerStateProbeSql = + "SELECT has_alter_server_state = has_perms_by_name(NULL, NULL, 'ALTER SERVER STATE');"; +}