From 5916fab68e84d2cd1f808f7bbea01db0fe1cf49a Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 18 Sep 2026 19:27:59 -0400 Subject: [PATCH] The automatic force-plan bot judged through the state-less gate and could not see that automatic plan correction owned, had failed on, or had withdrawn the plan: it now reads the store's forcing and APC state once per pass, judges through the whole gate plus two blockers of its own, fails closed on an unknown state, stands down where FORCE_LAST_GOOD_PLAN is on, and journals every blocker with its evidence (#3654) PlanForceBot called FactRemediation.ForcePlanBlockers(target) - the one-argument overload #3652 kept for compatibility - so the automatic actor's gate was the two #2138 blockers only, and on the five live targets #3652 cross-referenced it would have judged all five unblocked. The bot now reads the pass's candidate targets' ForcePlanTargetState in ONE batched statement through the store seam (IPlanForceActionStore.TryGetTargetStatesAsync over DarlingForcePlanTargetStateReader, under the bot's own PostAnalysisForcePlanSeconds deadline) and judges each through ForcePlanBotPolicy.Blockers: the two-argument gate's list verbatim, then apc_enabled_for_database (FLGP ON for the database - the engine is the forcer there) and state_unavailable (read failed, no row for the key, or read ran and observed nothing - not even the enablement row the collector writes for every database it sees). The journal's reasons column carries the names; detail carries name: evidence one per line, so a blocked row is auditable against the snapshot it was judged on. The Lite.Tests gap pin flips to a closure pin; Lite has no bot. --- .../Darling.Tests/ForcePlanBotPolicyTests.cs | 180 ++++++++++ Darling/Darling.Tests/PlanForceBotTests.cs | 316 +++++++++++++++++- .../StragglerCommandTimeoutTests.cs | 8 +- .../Mcp/DarlingForcePlanTargetStateReader.cs | 12 +- .../PgPlanForceActionStore.cs | 60 +++- .../PlanForceBot.cs | 155 ++++++--- Lite.Tests/ForcePlanTargetStateTests.cs | 43 ++- .../FactRemediation.cs | 11 +- .../ForcePlanBotPolicy.cs | 122 +++++++ 9 files changed, 847 insertions(+), 60 deletions(-) diff --git a/Darling/Darling.Tests/ForcePlanBotPolicyTests.cs b/Darling/Darling.Tests/ForcePlanBotPolicyTests.cs index 12c566c0fc..7c6299dab6 100644 --- a/Darling/Darling.Tests/ForcePlanBotPolicyTests.cs +++ b/Darling/Darling.Tests/ForcePlanBotPolicyTests.cs @@ -7,6 +7,7 @@ */ using System; +using System.Linq; using PerformanceMonitor.Analysis; using Xunit; @@ -198,6 +199,185 @@ public void BothGatesOpen_IsTheOnlyPathToForce() Assert.Empty(decision.Reasons); } + /* ---------------- the bot's whole blocker list (#3654) ---------------- */ + + private static readonly DateTime Observed = new(2026, 9, 18, 12, 0, 0, DateTimeKind.Utc); + + private static ForcePlanTargetState Clean(string? flgp = "OFF", DateTime? enablementAt = null) => new( + PlanIsForced: null, PlanForcingType: null, ForceFailureCount: null, LastForceFailureReason: null, + PlanObservedAtUtc: null, + OtherForcedPlanId: null, OtherForcedPlanForcingType: null, OtherForcedPlanObservedAtUtc: null, + ApcState: null, ApcStateReason: null, ApcRegressedPlanId: null, ApcLastGoodPlanId: null, + ApcLastGoodPlanForcingType: null, ApcLastGoodPlanIsForced: null, ApcLastGoodPlanForceFailureReason: null, + ApcExecuteActionInitiatedBy: null, ApcObservedAtUtc: null, + ForceLastGoodPlanActualState: flgp, EnablementObservedAtUtc: enablementAt ?? (flgp is null ? null : Observed)); + + [Fact] + public void Blockers_IsTheWholeSharedGate_PlusNothing_WhenTheStateIsCleanAndObserved() + { + /* The happy path: state read, FLGP OFF, nothing forced, no recommendation. The bot's list is + exactly the two-argument gate's (empty here) — no bot-only blocker fires on a clean read. */ + var blockers = ForcePlanBotPolicy.Blockers(Target(), Clean(), stateUnavailableReason: null); + + Assert.Empty(blockers); + Assert.Empty(ForcePlanBotPolicy.Names(blockers)); + Assert.Null(ForcePlanBotPolicy.Evidence(blockers)); + } + + [Fact] + public void Blockers_CarriesTheSharedGatesVerdict_Verbatim() + { + /* Same function, same names, same evidence strings as structured_remediation — the bot never + recomputes the gate (#2146). APC AUTO-forced on the target plan, plus the target's own PSP flag. */ + var state = Clean() with + { + PlanIsForced = true, PlanForcingType = "AUTO", ForceFailureCount = 0, PlanObservedAtUtc = Observed, + ApcState = "Verifying", ApcLastGoodPlanId = 7, ApcObservedAtUtc = Observed, + }; + var target = Target(psp: true); + + var shared = FactRemediation.ForcePlanBlockers(target, state); + var bot = ForcePlanBotPolicy.Blockers(target, state, stateUnavailableReason: null); + + Assert.Equal(shared, bot); + Assert.Equal(new[] { "parameter_sensitivity_cofired", "apc_owns_it" }, ForcePlanBotPolicy.Names(bot)); + } + + [Fact] + public void Blockers_FlgpOn_AddsTheStandDown_AfterTheSharedGate() + { + var state = Clean(flgp: "ON") with + { + PlanIsForced = true, PlanForcingType = "AUTO", PlanObservedAtUtc = Observed, + }; + + var blockers = ForcePlanBotPolicy.Blockers(Target(), state, stateUnavailableReason: null); + + Assert.Equal(new[] { "apc_owns_it", ForcePlanBotPolicy.ReasonApcEnabledForDatabase }, ForcePlanBotPolicy.Names(blockers)); + var standDown = blockers[1]; + Assert.Contains("force_last_good_plan_actual_state = ON for orders at 2026-09-18T12:00:00Z", standDown.Evidence, StringComparison.Ordinal); + Assert.Contains("second forcer", standDown.Evidence, StringComparison.Ordinal); + } + + [Theory] + [InlineData("ON")] + [InlineData("on")] + [InlineData("On")] + public void Blockers_FlgpOn_IsCaseInsensitive_OnTheStoredSpelling(string spelling) + { + var blockers = ForcePlanBotPolicy.Blockers(Target(), Clean(flgp: spelling), stateUnavailableReason: null); + + Assert.Equal(new[] { ForcePlanBotPolicy.ReasonApcEnabledForDatabase }, ForcePlanBotPolicy.Names(blockers)); + } + + [Theory] + [InlineData("OFF")] + [InlineData("off")] + public void Blockers_FlgpOff_FiresNothing(string spelling) + { + Assert.Empty(ForcePlanBotPolicy.Blockers(Target(), Clean(flgp: spelling), stateUnavailableReason: null)); + } + + [Fact] + public void Blockers_NullState_IsUnavailable_QuotingTheReadersReason() + { + var blockers = ForcePlanBotPolicy.Blockers(Target(), state: null, stateUnavailableReason: "the read failed (NpgsqlException: timeout)"); + + var only = Assert.Single(blockers); + Assert.Equal(ForcePlanBotPolicy.ReasonStateUnavailable, only.Name); + Assert.StartsWith("the read failed (NpgsqlException: timeout) — an unattended force cannot proceed", only.Evidence, StringComparison.Ordinal); + } + + [Fact] + public void Blockers_NullState_WithNoReason_IsUnavailable_NamingTheMissingKey() + { + var blockers = ForcePlanBotPolicy.Blockers(Target(), state: null, stateUnavailableReason: null); + + var only = Assert.Single(blockers); + Assert.Equal(ForcePlanBotPolicy.ReasonStateUnavailable, only.Name); + Assert.Contains("returned no row for plan 7 of query 42 in orders", only.Evidence, StringComparison.Ordinal); + } + + [Fact] + public void Blockers_NullState_StillCarriesTheTargetHalf_First() + { + /* Unknown state does not erase what the target itself says: PSP is named, then the + unavailability. */ + var blockers = ForcePlanBotPolicy.Blockers(Target(psp: true), state: null, stateUnavailableReason: "boom"); + + Assert.Equal(new[] { "parameter_sensitivity_cofired", ForcePlanBotPolicy.ReasonStateUnavailable }, ForcePlanBotPolicy.Names(blockers)); + } + + [Fact] + public void Blockers_EmptyState_IsUnavailable_BecauseTheEnablementHalfIsTheCollectorsEveryDatabaseRow() + { + var empty = Clean(flgp: null); + Assert.True(empty.IsEmpty); + + var blockers = ForcePlanBotPolicy.Blockers(Target(), empty, stateUnavailableReason: null); + + var only = Assert.Single(blockers); + Assert.Equal(ForcePlanBotPolicy.ReasonStateUnavailable, only.Name); + Assert.Contains("observed nothing for this target inside the last 24 hours", only.Evidence, StringComparison.Ordinal); + Assert.Contains("no plan_correction capture for orders at all", only.Evidence, StringComparison.Ordinal); + } + + [Fact] + public void Blockers_APlanNotObservedButADatabaseThatIs_IsNotUnavailable() + { + /* The ordinary shape for a best plan that is not executing: no query_store_stats row for it, no + recommendation — but the enablement row is there (OFF), so the store CAN see the database and + the bot proceeds. Null on a half means not observed, not unavailable. */ + var state = Clean(flgp: "OFF"); + Assert.False(state.IsEmpty); + + Assert.Empty(ForcePlanBotPolicy.Blockers(Target(), state, stateUnavailableReason: null)); + } + + [Fact] + public void Evidence_IsOneLinePerBlocker_NameColonEvidence() + { + var blockers = ForcePlanBotPolicy.Blockers(Target(psp: true), Clean(flgp: "ON"), stateUnavailableReason: null); + + var detail = ForcePlanBotPolicy.Evidence(blockers); + Assert.NotNull(detail); + var lines = detail!.Split('\n'); + Assert.Equal(2, lines.Length); + Assert.StartsWith("parameter_sensitivity_cofired: ", lines[0], StringComparison.Ordinal); + Assert.StartsWith("apc_enabled_for_database: ", lines[1], StringComparison.Ordinal); + } + + [Fact] + public void TheBotsOwnReasons_AreNotInTheSharedVocabulary() + { + /* They belong to the actor, not the advice: an agent reading structured_remediation must never + see state_unavailable or apc_enabled_for_database as a blocker, because for a reader neither + IS one. Pinned as strings because both are consumer API. */ + Assert.Equal("state_unavailable", ForcePlanBotPolicy.ReasonStateUnavailable); + Assert.Equal("apc_enabled_for_database", ForcePlanBotPolicy.ReasonApcEnabledForDatabase); + + var shared = typeof(ForcePlanBlockerNames).GetFields() + .Select(f => (string)f.GetValue(null)!) + .ToArray(); + Assert.DoesNotContain(ForcePlanBotPolicy.ReasonStateUnavailable, shared); + Assert.DoesNotContain(ForcePlanBotPolicy.ReasonApcEnabledForDatabase, shared); + } + + [Fact] + public void Evaluate_WithTheBotsBlockers_IsBlocked_EvenWithEveryGateOpen() + { + /* End to end through the decision table: the state names apc_enabled_for_database, and no + combination of gates turns that into WouldForce or Force. */ + var blockers = ForcePlanBotPolicy.Blockers(Target(), Clean(flgp: "ON"), stateUnavailableReason: null); + + var decision = ForcePlanBotPolicy.Evaluate( + Target(), ForcePlanBotPolicy.Names(blockers), serverOptedIn: true, Enabled(dryRun: false), + ForcePlanBotHistory.Empty, Now); + + Assert.Equal(ForcePlanBotDecisionKind.Blocked, decision.Kind); + Assert.Equal(new[] { ForcePlanBotPolicy.ReasonApcEnabledForDatabase }, decision.Reasons); + } + /* ---------------- settings hygiene ---------------- */ [Fact] diff --git a/Darling/Darling.Tests/PlanForceBotTests.cs b/Darling/Darling.Tests/PlanForceBotTests.cs index 8421ef40b5..6c8261c982 100644 --- a/Darling/Darling.Tests/PlanForceBotTests.cs +++ b/Darling/Darling.Tests/PlanForceBotTests.cs @@ -30,6 +30,11 @@ namespace Darling.Tests; /// takes none, and PlanForceNoWritePathTests pins that the build ships no implementation of /// the seam and no force/unforce/evict statement anywhere in the service assembly. The write path's /// own orchestration tests arrive with it (#2731). +/// +/// #3654 added the gate's STATE half to the bot: the fake store answers the batched +/// forcing/APC state read too, so the five #3652 blockers, the unavailable-state arm and the FLGP +/// stand-down are driven end to end through the orchestrator and pinned on what it JOURNALS — names in +/// reasons, evidence in detail. /// public sealed class PlanForceBotTests { @@ -43,6 +48,16 @@ private sealed class FakeStore : IPlanForceActionStore public ForcePlanBotHistory History { get; set; } = ForcePlanBotHistory.Empty; private long _nextId = 1; + /* The state half (#3654). Default: the read succeeds and every target it is asked about comes + back CLEAN and observed (FLGP OFF, nothing forced, no recommendation) — the shape a bot on a + healthy, APC-off server sees — so the pre-#3654 tests keep their meaning. A test overrides + per key, or fails the read, or returns an empty dictionary to simulate a missing key. */ + public Dictionary States { get; } = new(ForcePlanTargetKey.Comparer); + public string? StateReadFailure { get; set; } + public bool ReturnOnlyExplicitStates { get; set; } + public int StateReads { get; private set; } + public List> StateReadTargets { get; } = new(); + public Task JournalAsync(PlanForceActionRecord record, CancellationToken ct) { var id = _nextId++; @@ -53,8 +68,97 @@ public Task JournalAsync(PlanForceActionRecord record, CancellationToken c public Task GetQueryHistoryAsync( int serverId, string database, long queryId, ForcePlanBotSettings settings, DateTime nowUtc, CancellationToken ct) => Task.FromResult(History); + + public Task<(IReadOnlyDictionary? States, string? UnavailableReason)> TryGetTargetStatesAsync( + int serverId, IReadOnlyList targets, DateTime nowUtc, CancellationToken ct) + { + StateReads++; + StateReadTargets.Add(targets); + + if (StateReadFailure is not null) + { + return Task.FromResult<(IReadOnlyDictionary?, string?)>((null, StateReadFailure)); + } + + var result = new Dictionary(ForcePlanTargetKey.Comparer); + foreach (var t in targets) + { + var key = ForcePlanTargetKey.Of(t); + if (States.TryGetValue(key, out var s)) + { + result[key] = s; + } + else if (!ReturnOnlyExplicitStates) + { + result[key] = Clean(); + } + } + + return Task.FromResult<(IReadOnlyDictionary?, string?)>((result, null)); + } } + /* ---------------- state shapes ---------------- */ + + private static readonly DateTime Observed = new(2026, 9, 18, 12, 0, 0, DateTimeKind.Utc); + + /// Read, observed, nothing in the way: FLGP OFF for the database, no forced plan, no + /// recommendation. The one shape that lets the bot through. + private static ForcePlanTargetState Clean(string flgp = "OFF") => new( + PlanIsForced: null, PlanForcingType: null, ForceFailureCount: null, LastForceFailureReason: null, + PlanObservedAtUtc: null, + OtherForcedPlanId: null, OtherForcedPlanForcingType: null, OtherForcedPlanObservedAtUtc: null, + ApcState: null, ApcStateReason: null, ApcRegressedPlanId: null, ApcLastGoodPlanId: null, + ApcLastGoodPlanForcingType: null, ApcLastGoodPlanIsForced: null, ApcLastGoodPlanForceFailureReason: null, + ApcExecuteActionInitiatedBy: null, ApcObservedAtUtc: null, + ForceLastGoodPlanActualState: flgp, EnablementObservedAtUtc: Observed); + + /// Read ran, saw nothing at all — not even the enablement row. + private static ForcePlanTargetState Empty() => Clean() with + { + ForceLastGoodPlanActualState = null, + EnablementObservedAtUtc = null, + }; + + /* The five live #3652 shapes, against the test target (plan 7 of query 42 in orders). */ + + /// APC mid-verification on exactly the proposed plan: AUTO-forced on plan 7. + private static ForcePlanTargetState ApcOwnsIt() => Clean() with + { + PlanIsForced = true, PlanForcingType = "AUTO", ForceFailureCount = 0, LastForceFailureReason = "NONE", + PlanObservedAtUtc = Observed, + ApcState = "Verifying", ApcRegressedPlanId = 9, ApcLastGoodPlanId = 7, + ApcLastGoodPlanForcingType = "AUTO", ApcLastGoodPlanIsForced = true, ApcObservedAtUtc = Observed, + }; + + /// An operator already forced plan 7 by hand. + private static ForcePlanTargetState AlreadyForced() => Clean() with + { + PlanIsForced = true, PlanForcingType = "MANUAL", ForceFailureCount = 0, LastForceFailureReason = "NONE", + PlanObservedAtUtc = Observed, + }; + + /// Forcing plan 7 is already failing on this server (the Forced Plan Failing counter). + private static ForcePlanTargetState ForcingFailed() => Clean() with + { + PlanIsForced = true, PlanForcingType = "MANUAL", ForceFailureCount = 12, + LastForceFailureReason = "NO_INDEX", PlanObservedAtUtc = Observed, + }; + + /// The engine recommended plan 7 and withdrew it: Expired / TempTableChanged. + private static ForcePlanTargetState ApcWithdrewIt() => Clean() with + { + ApcState = "Expired", ApcStateReason = "TempTableChanged", ApcRegressedPlanId = 9, ApcLastGoodPlanId = 7, + ApcObservedAtUtc = Observed, + }; + + /// The engine already resolved the query's regression through plan 5, not 7. + private static ForcePlanTargetState ApcResolvedDifferently() => Clean() with + { + ApcState = "Success", ApcStateReason = "LastGoodPlanForced", ApcRegressedPlanId = 9, ApcLastGoodPlanId = 5, + ApcLastGoodPlanForcingType = "AUTO", ApcLastGoodPlanIsForced = true, ApcObservedAtUtc = Observed, + }; + /* ---------------- scaffolding ---------------- */ private static ServerRuntime Runtime(string engine = "sqlserver") => new() @@ -180,6 +284,212 @@ public async Task TargetsAreDedupedAcrossFindings_AndCappedPerPass() Assert.Equal(store.Journaled.Count, store.Journaled.Select(r => r.QueryId).Distinct().Count()); } + /* ---------------- the state half (#3654) ---------------- */ + + [Fact] + public async Task TheStateIsReadOnce_ForTheWholePass_BeforeAnyTargetIsJudged() + { + /* One batched statement per pass, sized by the same cap that bounds the history reads — not + one per target. Fourteen distinct queries, cap ten: the read is asked about exactly the ten + the pass will judge, once. */ + var (bot, store) = Build(Enabled()); + var finding = Finding(Enumerable.Range(1, 14).Select(q => Target(queryId: q)).ToArray()); + + await bot.RunAfterAnalysisAsync(Runtime(), Config(), new[] { finding }, CancellationToken.None); + + Assert.Equal(1, store.StateReads); + var asked = Assert.Single(store.StateReadTargets); + Assert.Equal(PlanForceBot.MaxTargetsPerPass, asked.Count); + Assert.Equal(Enumerable.Range(1, 10).Select(q => (long)q), asked.Select(t => t.QueryId)); + } + + [Fact] + public async Task NoCandidates_MeansNoStateRead() + { + /* A pass with no PLAN_REGRESSION targets must not spend the store round trip. */ + var (bot, store) = Build(Enabled()); + var other = new AnalysisFinding { ServerId = ServerId, Remediation = new RemediationAction("BLOCKING", "none", Array.Empty()) }; + + await bot.RunAfterAnalysisAsync(Runtime(), Config(), new[] { other }, CancellationToken.None); + + Assert.Equal(0, store.StateReads); + Assert.Empty(store.Journaled); + } + + public static IEnumerable TheFiveLiveShapes() + { + yield return new object[] { "apc_owns_it", ApcOwnsIt(), "plan_forcing_type = AUTO" }; + yield return new object[] { "already_forced", AlreadyForced(), "plan_forcing_type = MANUAL" }; + yield return new object[] { "forcing_failed_on_this_plan", ForcingFailed(), "force_failure_count = 12, last_force_failure_reason = NO_INDEX" }; + yield return new object[] { "apc_withdrew_it", ApcWithdrewIt(), "Expired / TempTableChanged" }; + yield return new object[] { "apc_resolved_differently", ApcResolvedDifferently(), "last_good_plan_id = 5 (not 7)" }; + } + + [Theory] + [MemberData(nameof(TheFiveLiveShapes))] + public async Task EachOfTheFiveStateBlockers_StopsTheForce_AndJournalsNameAndEvidence( + string blocker, ForcePlanTargetState state, string evidenceFragment) + { + /* Every gate open — enabled, live, server opted in — and the target's own evidence is clean + (no PSP, primary). Before #3654 each of these journaled as a WITHHELD force. Now the store's + state stops it, the row names the blocker in reasons (the consumer API, comma-joined) and + quotes the store values it was built from in detail, stamped with the snapshot time. */ + var (bot, store) = Build(Enabled(dryRun: false)); + store.States[new ForcePlanTargetKey("orders", 42, 7)] = state; + + await bot.RunAfterAnalysisAsync(Runtime(), Config(optedIn: true), + new[] { Finding(Target()) }, CancellationToken.None); + + var row = Assert.Single(store.Journaled); + Assert.Equal(PgPlanForceActionStore.ActionBlocked, row.Action); + Assert.NotEqual(PgPlanForceActionStore.ActionForce, row.Action); + Assert.Contains(blocker, row.Reasons.Split(',')); + Assert.NotNull(row.Detail); + Assert.Contains($"{blocker}: ", row.Detail, StringComparison.Ordinal); + Assert.Contains(evidenceFragment, row.Detail, StringComparison.Ordinal); + Assert.Contains("2026-09-18T12:00:00Z", row.Detail, StringComparison.Ordinal); + } + + [Fact] + public async Task AStateReadFailure_IsABlocker_NotAFault_AndTheJournalSaysWhy() + { + /* The advisory surface notes 'unknown' and lets a human cross-reference; an unattended writer + has nobody to hand the decision to, so unknown fails CLOSED. Every gate open, and the row is + blocked/state_unavailable with the reader's reason quoted — not would_force, not force. */ + var (bot, store) = Build(Enabled(dryRun: false)); + store.StateReadFailure = "the forcing and automatic-plan-correction state read failed (NpgsqlException: timeout)"; + + await bot.RunAfterAnalysisAsync(Runtime(), Config(optedIn: true), + new[] { Finding(Target()) }, CancellationToken.None); + + var row = Assert.Single(store.Journaled); + Assert.Equal(PgPlanForceActionStore.ActionBlocked, row.Action); + Assert.Equal(ForcePlanBotPolicy.ReasonStateUnavailable, row.Reasons); + Assert.Contains("NpgsqlException: timeout", row.Detail, StringComparison.Ordinal); + Assert.Contains("unattended force cannot proceed", row.Detail, StringComparison.Ordinal); + } + + [Fact] + public async Task AStateReadThatReturnsNoRowForTheTarget_IsAlsoUnavailable() + { + /* The reader LEFT JOINs from the requested keys so every key comes back — but the bot does not + rely on that: a key the dictionary lacks is judged unknown, not clean. */ + var (bot, store) = Build(Enabled(dryRun: false)); + store.ReturnOnlyExplicitStates = true; + + await bot.RunAfterAnalysisAsync(Runtime(), Config(optedIn: true), + new[] { Finding(Target()) }, CancellationToken.None); + + var row = Assert.Single(store.Journaled); + Assert.Equal(PgPlanForceActionStore.ActionBlocked, row.Action); + Assert.Equal(ForcePlanBotPolicy.ReasonStateUnavailable, row.Reasons); + Assert.Contains("returned no row for plan 7 of query 42 in orders", row.Detail, StringComparison.Ordinal); + } + + [Fact] + public async Task AnEmptyState_IsUnavailable_BecauseTheEnablementIsUnknown() + { + /* Read ran, observed nothing on any half — including the enablement row the plan-correction + collector writes for every database it can see. FORCE_LAST_GOOD_PLAN is unknown for the + database, so the FLGP stand-down cannot be evaluated, and the bot does not guess. */ + var (bot, store) = Build(Enabled(dryRun: false)); + store.States[new ForcePlanTargetKey("orders", 42, 7)] = Empty(); + + await bot.RunAfterAnalysisAsync(Runtime(), Config(optedIn: true), + new[] { Finding(Target()) }, CancellationToken.None); + + var row = Assert.Single(store.Journaled); + Assert.Equal(PgPlanForceActionStore.ActionBlocked, row.Action); + Assert.Equal(ForcePlanBotPolicy.ReasonStateUnavailable, row.Reasons); + Assert.Contains("no plan_correction capture for orders at all", row.Detail, StringComparison.Ordinal); + Assert.Contains("FORCE_LAST_GOOD_PLAN enablement is unknown", row.Detail, StringComparison.Ordinal); + } + + [Fact] + public async Task FlgpOn_StandsTheBotDown_ForTheDatabase_WhateverElseTheGateSays() + { + /* Automatic plan correction is ON for orders and has not touched query 42: no forced plan, no + recommendation, nothing for the shared gate to name. The bot still stands down — the engine is + the forcer on this database and two forcers on one database is the #3652 failure. The advice + only changes its verb here; the bot cannot be the operator who read the guidance. */ + var (bot, store) = Build(Enabled(dryRun: false)); + store.States[new ForcePlanTargetKey("orders", 42, 7)] = Clean(flgp: "ON"); + + await bot.RunAfterAnalysisAsync(Runtime(), Config(optedIn: true), + new[] { Finding(Target()) }, CancellationToken.None); + + var row = Assert.Single(store.Journaled); + Assert.Equal(PgPlanForceActionStore.ActionBlocked, row.Action); + Assert.Equal(ForcePlanBotPolicy.ReasonApcEnabledForDatabase, row.Reasons); + Assert.Contains("force_last_good_plan_actual_state = ON for orders at 2026-09-18T12:00:00Z", row.Detail, StringComparison.Ordinal); + } + + [Fact] + public async Task FlgpOn_WithApcVerifyingThisPlan_JournalsBothNames() + { + /* The expected shape on an APC database: the shared gate names apc_owns_it (with the store + evidence) and the bot adds its own stand-down. Both land, shared gate's first, so the row reads + the way structured_remediation does plus the bot's reason. */ + var (bot, store) = Build(Enabled(dryRun: false)); + store.States[new ForcePlanTargetKey("orders", 42, 7)] = ApcOwnsIt() with { ForceLastGoodPlanActualState = "ON" }; + + await bot.RunAfterAnalysisAsync(Runtime(), Config(optedIn: true), + new[] { Finding(Target()) }, CancellationToken.None); + + var row = Assert.Single(store.Journaled); + Assert.Equal(PgPlanForceActionStore.ActionBlocked, row.Action); + Assert.Equal("apc_owns_it,apc_enabled_for_database", row.Reasons); + Assert.Contains("apc_owns_it: query_store_stats: plan 7 is_forced_plan = true", row.Detail, StringComparison.Ordinal); + Assert.Contains("\napc_enabled_for_database: plan_correction: force_last_good_plan_actual_state = ON", row.Detail, StringComparison.Ordinal); + } + + [Fact] + public async Task TheTargetCarriedBlocker_StillRidesTheSameRow_WithEvidenceNow() + { + /* The two #2138 blockers did not change meaning; they now carry evidence in detail like the + five, off the same two-argument gate. */ + var (bot, store) = Build(Enabled(dryRun: false)); + + await bot.RunAfterAnalysisAsync(Runtime(), Config(optedIn: true), + new[] { Finding(Target(psp: true)) }, CancellationToken.None); + + var row = Assert.Single(store.Journaled); + Assert.Equal("parameter_sensitivity_cofired", row.Reasons); + Assert.Contains("parameter_sensitivity_cofired: the query also carries the PARAMETER_SENSITIVITY detector's plan-cache signature", row.Detail, StringComparison.Ordinal); + } + + [Fact] + public async Task ABlockFromTheBotsOwnHistoryGates_KeepsANullDetail() + { + /* failed_force_cooldown is the policy's own reason, not a blocker with store evidence; the row + carries the name and nothing to quote, exactly as before #3654. */ + var (bot, store) = Build(Enabled(dryRun: false)); + store.History = new ForcePlanBotHistory(null, 0, 2); + + await bot.RunAfterAnalysisAsync(Runtime(), Config(optedIn: true), + new[] { Finding(Target()) }, CancellationToken.None); + + var row = Assert.Single(store.Journaled); + Assert.Equal(PgPlanForceActionStore.ActionBlocked, row.Action); + Assert.Equal(ForcePlanBotPolicy.ReasonFailedForceCooldown, row.Reasons); + Assert.Null(row.Detail); + } + + [Fact] + public async Task TheCooldown_StillOutranksTheStateBlockers_SoABlockedRowIsNotRepeatedEveryPass() + { + /* The policy's ordering did not move: a target inside its cooldown is suppressed before the + blockers are consulted, whichever half they came from. */ + var (bot, store) = Build(Enabled(dryRun: false)); + store.States[new ForcePlanTargetKey("orders", 42, 7)] = ApcOwnsIt(); + store.History = new ForcePlanBotHistory(DateTime.UtcNow.AddHours(-1), 0, 0); + + await bot.RunAfterAnalysisAsync(Runtime(), Config(optedIn: true), + new[] { Finding(Target()) }, CancellationToken.None); + + Assert.Empty(store.Journaled); + } + /* ---------------- every switch open, and still no write ---------------- */ [Fact] @@ -189,7 +499,11 @@ public async Task EveryGateOpen_JournalsTheForceDecisionAsWithheld_BecauseThisBu per-server opt-in has opened every switch there is, the policy returns Force, and the bot still cannot touch the server. The row has to SAY that rather than quietly reading as a would-force, or the trail would tell an operator who believes the bot is live exactly what - a shadow-mode bot tells one who knows it is not. */ + a shadow-mode bot tells one who knows it is not. + + Since #3654 this is also the happy path of the state half: the fake's default state is read, + observed and clean (FLGP OFF, nothing forced, no recommendation), and the bot proceeds — the + new blockers stop contraindicated forces, not forcing. */ var (bot, store) = Build(Enabled(dryRun: false)); await bot.RunAfterAnalysisAsync(Runtime(), Config(optedIn: true), diff --git a/Darling/Darling.Tests/StragglerCommandTimeoutTests.cs b/Darling/Darling.Tests/StragglerCommandTimeoutTests.cs index ef74df1651..8e233516f8 100644 --- a/Darling/Darling.Tests/StragglerCommandTimeoutTests.cs +++ b/Darling/Darling.Tests/StragglerCommandTimeoutTests.cs @@ -410,12 +410,14 @@ public void ThePostAnalysisForcePlanDeadline_FitsInsideThePassItRidesOn() var seconds = ServiceCommandDeadlines.PostAnalysisForcePlanSeconds; /* Two commands per evaluated target — one history read, one journal write — sequentially, on - separate store connections. */ - var worstCase = 2 * PlanForceBot.MaxTargetsPerPass * seconds; + separate store connections; plus ONE for the pass — the batched forcing/APC state read (#3654), + which runs under this same constant through PgPlanForceActionStore.TryGetTargetStatesAsync and + is sized by the same cap, so it is one statement however many targets the pass holds. */ + var worstCase = (2 * PlanForceBot.MaxTargetsPerPass + 1) * seconds; Assert.True( worstCase <= DarlingWorker.AnalysisTimeout.TotalSeconds, - $"{PlanForceBot.MaxTargetsPerPass} targets x 2 commands x {seconds}s = {worstCase}s exceeds the " + $"{PlanForceBot.MaxTargetsPerPass} targets x 2 commands x {seconds}s + one batched state read at {seconds}s = {worstCase}s exceeds the " + $"{DarlingWorker.AnalysisTimeout.TotalSeconds}s the analysis pass itself is budgeted for. The " + "hook runs after that pass inside the same sweep body, holding a max_concurrent_sweeps permit " + "while the server's collection cannot relaunch, so it must not outlast the pass it rides on"); diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingForcePlanTargetStateReader.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingForcePlanTargetStateReader.cs index 1197f51641..42b1ada0b8 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingForcePlanTargetStateReader.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingForcePlanTargetStateReader.cs @@ -210,7 +210,7 @@ public static string BuildSql(int targetCount) try { - return (await ReadAsync(postgres, serverId, targets, nowUtc ?? DateTime.UtcNow, cancellationToken), null); + return (await ReadAsync(postgres, serverId, targets, nowUtc ?? DateTime.UtcNow, cancellationToken: cancellationToken), null); } catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { @@ -224,9 +224,15 @@ public static string BuildSql(int targetCount) /// Runs the statement. Distinct targets only (a finding can carry one key twice across /// replicas, #1882). + /// The deadline regime of the CALLER, not of this file: the MCP + /// tools take (the default), and the force-plan bot + /// (#3654) passes its own ServiceCommandDeadlines.PostAnalysisForcePlanSeconds, because the same + /// statement issued from a post-analysis hook holding a sweep permit is bounded by the pass it rides on, + /// not by an agent's patience. A timeout there is not a fault the bot recovers from — it journals + /// state_unavailable and forces nothing. public static async Task> ReadAsync( NpgsqlDataSource postgres, int serverId, IReadOnlyList targets, DateTime nowUtc, - CancellationToken cancellationToken = default) + int? commandTimeoutSeconds = null, CancellationToken cancellationToken = default) { var result = new Dictionary(ForcePlanTargetKey.Comparer); var keys = targets.Select(ForcePlanTargetKey.Of).Distinct(ForcePlanTargetKey.Comparer).ToList(); @@ -236,7 +242,7 @@ public static async Task -/// The slice of the journal the BOT consumes — a seam so the orchestrator is testable with an +/// The slice of the STORE the BOT consumes — a seam so the orchestrator is testable with an /// in-memory fake (the gate decisions in PlanForceBot are exactly the logic that must be -/// provable without a live store). Phase 1's bot journals and reads history; the read only a live +/// provable without a live store). Phase 1's bot journals, reads its history, and (#3654) reads the +/// store's forcing and automatic-plan-correction state for the pass's targets; the read only a live /// force can populate (GetPendingReviewsAsync) and the audit read /// (GetRecentActionsAsync) stay on the concrete class, where they are specced against a live /// store without widening the seam the orchestrator is written against. +/// +/// The state read sits on THIS seam rather than on a second one because the bot's every store +/// read must be fakeable from one object: the orchestrator's tests drive the five #3652 blockers, the +/// unavailable-state arm and the FLGP stand-down through the same fake that supplies its history, and +/// DarlingWorker keeps constructing the bot with the journal and nothing else. It is a read of the +/// monitoring store like the history read — never of a monitored server — so it belongs on the class +/// whose whole contract is "the monitoring store, from the bot's side". /// public interface IPlanForceActionStore { @@ -61,6 +70,17 @@ public interface IPlanForceActionStore Task GetQueryHistoryAsync( int serverId, string database, long queryId, ForcePlanBotSettings settings, DateTime nowUtc, CancellationToken ct); + + /// + /// What the store knows about each of ' forcing and APC state right now + /// (#3654) — one batched statement for the whole pass, keyed by , so a + /// pass of ten targets costs one store round trip for this half rather than ten. Returns + /// (null, reason) when the read failed; the bot treats that as its state_unavailable + /// blocker and quotes the reason into the journal. Never throws for a store fault — a cancellation + /// requested through is the one exception that propagates. + /// + Task<(IReadOnlyDictionary? States, string? UnavailableReason)> TryGetTargetStatesAsync( + int serverId, IReadOnlyList targets, DateTime nowUtc, CancellationToken ct); } /// @@ -259,6 +279,42 @@ AND pfa.action_time > $5 Convert.ToInt32(reader.GetValue(2), CultureInfo.InvariantCulture)); } + /// + /// over + /// — the SAME statement the MCP surfaces run (#3652), so the bot's state half and the advice's cannot + /// disagree from the same store — under the bot's deadline regime rather than the MCP one: this runs + /// inside the post-analysis hook, holding a sweep permit, and is bounded like every other command on + /// this class (; the pass-budget + /// arithmetic in StragglerCommandTimeoutTests counts it). A timeout or any other store fault is + /// returned as the reason, not thrown: for an unattended actor an unreadable state is a blocker with + /// evidence, and the journal row is where that evidence belongs. + /// + public async Task<(IReadOnlyDictionary? States, string? UnavailableReason)> TryGetTargetStatesAsync( + int serverId, IReadOnlyList targets, DateTime nowUtc, CancellationToken ct) + { + if (targets is not { Count: > 0 }) + { + return (new Dictionary(ForcePlanTargetKey.Comparer), null); + } + + try + { + var states = await DarlingForcePlanTargetStateReader.ReadAsync( + _postgres, serverId, targets, nowUtc, + commandTimeoutSeconds: ServiceCommandDeadlines.PostAnalysisForcePlanSeconds, + cancellationToken: ct); + return (states, null); + } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + throw; + } + catch (Exception ex) + { + return (null, $"the forcing and automatic-plan-correction state read failed ({ex.GetType().Name}: {ex.Message})"); + } + } + /// How long an 'attempting' intent row may stand alone before the review treats it as /// an orphan — long enough for any live force + completion write to finish, short enough that a /// force whose completion journal write failed is verified within the first checkpoint. diff --git a/Darling/PerformanceMonitor.Darling.Service/PlanForceBot.cs b/Darling/PerformanceMonitor.Darling.Service/PlanForceBot.cs index e8d5f89a29..9692097d75 100644 --- a/Darling/PerformanceMonitor.Darling.Service/PlanForceBot.cs +++ b/Darling/PerformanceMonitor.Darling.Service/PlanForceBot.cs @@ -18,10 +18,18 @@ namespace PerformanceMonitor.Darling.Service; /// /// The auto force-plan bot's orchestration (#2138 phase 1): runs after each scheduled analysis pass, /// re-judges the pass's force-plan targets through the SAME policy gate agents inspect on the MCP -/// surfaces ( feeding +/// surfaces (, +/// both halves, wrapped by and feeding /// ), and journals every decision to /// collect.plan_force_actions with the evidence that produced it. /// +/// The state half is read here, once per pass (#3654). The gate's second half needs what +/// the store knows about each target's forcing and automatic-plan-correction state NOW — the same +/// query_store_stats / plan_correction read the MCP tools make (#3652), batched into one +/// statement for the pass's whole candidate list through the store seam — and the bot judges each +/// target with it. Without it the bot's gate was the two #2138 blockers only, and on the five live +/// targets #3652 cross-referenced it would have judged all five unblocked. +/// /// Phase 1 cannot write to a monitored server, structurally. This class holds no /// — no field, no constructor parameter, no factory — and no /// implementation of that interface ships in this build (see the interface's remarks and @@ -45,9 +53,11 @@ pathological drill-down can never turn one analysis pass into a journal flood. It counts targets EVALUATED, not rows journaled, and that is the load-bearing choice: each evaluated target costs a store round trip (GetQueryHistoryAsync) whatever the verdict turns out - to be, so the cap bounds the WORK a pass can do, not just its output. Counting only journaled - rows would let a pass whose targets are all inside their cooldown spend an unbounded number of - history reads for an empty journal — the one shape the budget exists to stop. The cost is that + to be — plus, since #3654, the pass costs ONE more for all of them together (the batched state + read, sized by this same cap) — so the cap bounds the WORK a pass can do, not just its output. + Counting only journaled rows would let a pass whose targets are all inside their cooldown spend + an unbounded number of history reads for an empty journal — the one shape the budget exists to + stop. The cost is that a suppressed target can occupy a slot a later actionable one wanted; that is acceptable because FactRemediation.ExtractPlanRegressionTargets already caps each finding at 5 targets ordered worst-regression-first, so the actionable ones are at the front of the list by construction. If @@ -127,8 +137,104 @@ private async Task EvaluateTargetsAsync( } var nowUtc = DateTime.UtcNow; + var candidates = CollectCandidates(findings); + if (candidates.Count == 0) + { + return; + } + + /* The gate's STATE half, read once for the whole pass (#3654). One statement for every candidate + rather than one per target — the read is one server's day of query_store_stats hashed on the + targets, and ten of those where one would do is the shape the per-pass cap exists to prevent. + Read BEFORE the per-target loop so every target in the pass is judged against the same snapshot; + a failure comes back as a reason, never an exception, because for this caller an unreadable + state is a verdict (below), not a fault to log and skip past. */ + var (states, stateUnavailableReason) = await _store.TryGetTargetStatesAsync( + runtime.ServerId, candidates, nowUtc, ct); + + foreach (var target in candidates) + { + ForcePlanTargetState? state = null; + if (states is not null) + { + states.TryGetValue(ForcePlanTargetKey.Of(target), out state); + } + + /* THE shared gate, whole: the same two-argument FactRemediation.ForcePlanBlockers that fills + structured_remediation's blockers and blocker_evidence on the MCP surfaces (#3652), never + recomputed locally, so advise and act cannot drift — plus the two blockers only a bot + needs, which ForcePlanBotPolicy.Blockers adds on top. + + Why this caller fails CLOSED where the advice merely notes (#3654, #3652). The advisory + surface, handed a state it could not read, writes `state_note: unknown` on the target and + leaves `eligible` as the finding-only verdict, because its reader is a human or an agent + who will cross-reference get_plan_corrections and sys.query_store_plan before running + anything — #3652 was found exactly that way. This caller has no reader between its verdict + and the write: with every gate open, Force is sp_query_store_force_plan on a production + server with nobody looking. The five #3652 blockers are the facts that would have stopped + five contraindicated forces, and every one of them fires only on an OBSERVED value, so an + unread state does not merely weaken the gate, it reproduces the pre-#3652 gate exactly. So + a null state (the read failed, or returned no row for this key) and an empty one (the read + ran and observed nothing — not even the enablement row the plan-correction collector + writes for every database it can see, so FORCE_LAST_GOOD_PLAN is unknown here) are both a + blocker with the reason as evidence, and the journal says `state_unavailable` rather than + a would_force row that nothing checked. Unknown is a note for a reader and a NO for an + actor. Likewise FLGP-on: the advice changes its verb and leaves the statement for an + operator who has read the guidance; the bot stands down for the whole database + (`apc_enabled_for_database`), because two forcers on one database is the failure #3652 + documented and the bot cannot be the one who read the guidance. */ + var blockers = ForcePlanBotPolicy.Blockers( + target, state, states is null ? stateUnavailableReason : null); + var history = await _store.GetQueryHistoryAsync( + runtime.ServerId, target.Database, target.QueryId, _settings, nowUtc, ct); + + var decision = ForcePlanBotPolicy.Evaluate( + target, ForcePlanBotPolicy.Names(blockers), currentConfig.PlanForceBotEnabled, _settings, + history, nowUtc); + + switch (decision.Kind) + { + case ForcePlanBotDecisionKind.Suppressed: + continue; + + case ForcePlanBotDecisionKind.Blocked: + /* reasons = the blocker NAMES (the consumer API, comma-joined as ever); detail = each + name with the evidence it was built from, one per line, so a blocked row is + auditable against the snapshot it was judged on (#3654). The policy returns the + blocker list as its reasons whenever the list is non-empty; when it is empty the + Blocked came from the bot's own history gates (failed-force memory, daily budget), + which carry no evidence beyond their name, and detail stays null as before. */ + await _store.JournalAsync(BuildRecord( + runtime, target, PgPlanForceActionStore.ActionBlocked, decision.Reasons, + PgPlanForceActionStore.OutcomeLogged, + detail: ForcePlanBotPolicy.Evidence(blockers), nowUtc), ct); + continue; + + case ForcePlanBotDecisionKind.WouldForce: + await _store.JournalAsync(BuildRecord( + runtime, target, PgPlanForceActionStore.ActionWouldForce, decision.Reasons, + PgPlanForceActionStore.OutcomeLogged, detail: null, nowUtc), ct); + continue; + + case ForcePlanBotDecisionKind.Force: + await JournalWithheldForceAsync(runtime, target, nowUtc, ct); + continue; + } + } + } + + /// + /// The pass's candidate list: PLAN_REGRESSION targets in finding order, one per (database, query) + /// across findings (a query regressed in two findings is one decision), cut at + /// . Materialized up front (rather than judged as it is walked, as it + /// was before #3654) so the state read can be ONE statement over the whole list; the order and the + /// cap are exactly what the walk produced, so the worst-regression-first assumption the cap rests on + /// is unchanged. + /// + private static List CollectCandidates(IReadOnlyList findings) + { var seen = new HashSet<(string Database, long QueryId)>(); - var evaluated = 0; + var candidates = new List(MaxTargetsPerPass); foreach (var finding in findings) { @@ -140,9 +246,9 @@ private async Task EvaluateTargetsAsync( foreach (var target in targets) { - if (evaluated >= MaxTargetsPerPass) + if (candidates.Count >= MaxTargetsPerPass) { - return; + return candidates; } if (!seen.Add((target.Database, target.QueryId))) @@ -150,40 +256,11 @@ private async Task EvaluateTargetsAsync( continue; } - evaluated++; - - /* THE shared gate: the same function that fills structured_remediation's blockers - on the MCP surfaces. Never recomputed locally, so advise and act cannot drift. */ - var blockers = FactRemediation.ForcePlanBlockers(target); - var history = await _store.GetQueryHistoryAsync( - runtime.ServerId, target.Database, target.QueryId, _settings, nowUtc, ct); - - var decision = ForcePlanBotPolicy.Evaluate( - target, blockers, currentConfig.PlanForceBotEnabled, _settings, history, nowUtc); - - switch (decision.Kind) - { - case ForcePlanBotDecisionKind.Suppressed: - continue; - - case ForcePlanBotDecisionKind.Blocked: - await _store.JournalAsync(BuildRecord( - runtime, target, PgPlanForceActionStore.ActionBlocked, decision.Reasons, - PgPlanForceActionStore.OutcomeLogged, detail: null, nowUtc), ct); - continue; - - case ForcePlanBotDecisionKind.WouldForce: - await _store.JournalAsync(BuildRecord( - runtime, target, PgPlanForceActionStore.ActionWouldForce, decision.Reasons, - PgPlanForceActionStore.OutcomeLogged, detail: null, nowUtc), ct); - continue; - - case ForcePlanBotDecisionKind.Force: - await JournalWithheldForceAsync(runtime, target, nowUtc, ct); - continue; - } + candidates.Add(target); } } + + return candidates; } /// diff --git a/Lite.Tests/ForcePlanTargetStateTests.cs b/Lite.Tests/ForcePlanTargetStateTests.cs index f6f4374a41..c01c3f94d8 100644 --- a/Lite.Tests/ForcePlanTargetStateTests.cs +++ b/Lite.Tests/ForcePlanTargetStateTests.cs @@ -396,16 +396,43 @@ public void VerifySql_SpeaksTheBlockersVocabulary() } [Fact] - public void ThePlanForceBotsGate_IsTheOriginalOverload_AndStillAgreesWithTheTargetHalf() + public void ThePlanForceBotsGate_IsTheWholeGate_PlusItsOwnTwo_AndTheOverloadsStillAgreeOnTheTargetHalf() { - /* PlanForceBot consults the one-argument overload, which has no state and therefore none of the - #3652 blockers — #3654 carries that gap; this pin is so it is a test, not a - surprise. The two overloads agree on the target-carried half. */ - var t = Target(psp: true, replica: "Secondary"); + /* Until #3654 this pinned the GAP: PlanForceBot consulted the one-argument overload, which has + no state and therefore none of the #3652 blockers. Now it pins the closure: the bot judges + through ForcePlanBotPolicy.Blockers, which is the two-argument gate's list verbatim (same + names, same evidence — what an agent reads is what the bot enforces) plus the two blockers + only an unattended actor needs. On the live #3652 shape — APC verifying its own AUTO force on + exactly the proposed plan — the one-argument overload still says nothing, and the bot's gate + says apc_owns_it. */ + var t = Target(); + var apcVerifying = State(planForced: true, forcingType: "AUTO", failures: 0, + apcState: "Verifying", apcLastGood: 99, apcLastGoodForcingType: "AUTO", apcLastGoodForced: true, flgp: "OFF"); + + Assert.Empty(FactRemediation.ForcePlanBlockers(t)); + var shared = FactRemediation.ForcePlanBlockers(t, apcVerifying); + var bot = ForcePlanBotPolicy.Blockers(t, apcVerifying, stateUnavailableReason: null); + Assert.Equal(new[] { ForcePlanBlockerNames.ApcOwnsIt }, shared.Select(b => b.Name)); + Assert.Equal(shared, bot); + + /* The bot's own two ride AFTER the shared gate's and never appear in the shared vocabulary: + FLGP on for the database stands the bot down where the advice only changes its verb, and an + unreadable state is a blocker for the bot where the advice writes a note. */ + var flgpOn = apcVerifying with { ForceLastGoodPlanActualState = "ON" }; + Assert.Equal( + new[] { ForcePlanBlockerNames.ApcOwnsIt, ForcePlanBotPolicy.ReasonApcEnabledForDatabase }, + ForcePlanBotPolicy.Blockers(t, flgpOn, null).Select(b => b.Name)); + Assert.Equal("on", Project(t, flgpOn).ApcMode); + Assert.Equal( + new[] { ForcePlanBotPolicy.ReasonStateUnavailable }, + ForcePlanBotPolicy.Blockers(t, null, "read failed").Select(b => b.Name)); + Assert.StartsWith("unknown: read failed", Project(t, null, "read failed").StateNote, StringComparison.Ordinal); + + /* The two overloads still agree on the target-carried half. */ + var carried = Target(psp: true, replica: "Secondary"); Assert.Equal( - FactRemediation.ForcePlanBlockers(t), - FactRemediation.ForcePlanBlockers(t, null).Select(b => b.Name).ToList()); - Assert.Empty(FactRemediation.ForcePlanBlockers(Target())); + FactRemediation.ForcePlanBlockers(carried), + FactRemediation.ForcePlanBlockers(carried, null).Select(b => b.Name).ToList()); } /* ------------------------------------------------------------------ 5. the reader SQL, both SKUs */ diff --git a/PerformanceMonitor.Analysis/FactRemediation.cs b/PerformanceMonitor.Analysis/FactRemediation.cs index e08797a91b..7552764da6 100644 --- a/PerformanceMonitor.Analysis/FactRemediation.cs +++ b/PerformanceMonitor.Analysis/FactRemediation.cs @@ -947,8 +947,10 @@ private static void AppendParameterSensitivityCaution(StringBuilder sb, ForcePla /// plan_correction (#1952). So the verdict now has two halves: this one, from the target, and /// , from a read-time /// — the pure function stays pure and pinnable, the caller reads the - /// state. This overload remains for the bot (PlanForceBot), which does not yet read the state - /// and must not auto-force until it does — see #3654. + /// state. This overload remains as the target-carried half on its own (tests pin the two halves + /// agree on it); nothing that acts consults it — the bot (PlanForceBot) reads the state once + /// per pass and judges through , the whole gate plus its + /// own two, since #3654. /// /// parameter_sensitivity_cofired — the query's plan-cache history shows the PSP /// signature (#2140); forcing pins ONE shape for every parameter value, so the "best" plan may be @@ -1140,8 +1142,9 @@ private static bool IsAutoForcing(string? forcingType) => string.Equals(forcingType, ForcePlanTargetState.ForcingTypeAuto, StringComparison.OrdinalIgnoreCase); /// The snapshot stamp every blocker's evidence ends with — ISO-8601 UTC, or a stated - /// absence, never an empty string that could read as "now". - private static string Stamp(DateTime? observedAtUtc) => + /// absence, never an empty string that could read as "now". Internal so the bot's own blockers + /// (, #3654) stamp their evidence the same way. + internal static string Stamp(DateTime? observedAtUtc) => observedAtUtc is DateTime t ? DateTime.SpecifyKind(t, DateTimeKind.Utc).ToString("yyyy-MM-dd'T'HH:mm:ss'Z'", CultureInfo.InvariantCulture) : "(no snapshot time)"; diff --git a/PerformanceMonitor.Analysis/ForcePlanBotPolicy.cs b/PerformanceMonitor.Analysis/ForcePlanBotPolicy.cs index 4186c54a85..8345eed43c 100644 --- a/PerformanceMonitor.Analysis/ForcePlanBotPolicy.cs +++ b/PerformanceMonitor.Analysis/ForcePlanBotPolicy.cs @@ -8,6 +8,7 @@ using System; using System.Collections.Generic; +using System.Linq; namespace PerformanceMonitor.Analysis; @@ -202,6 +203,127 @@ public static class ForcePlanBotPolicy public const string ReasonDryRun = "dry_run"; public const string ReasonServerNotOptedIn = "server_not_opted_in"; + /* The bot's OWN two blockers (#3654) — on the policy, not in FactRemediation's shared vocabulary, + because only an unattended actor needs them. The advisory surface can say "unknown" and hand + the decision to a human who will cross-reference; a bot has no one to hand it to. */ + + /// The store's forcing and automatic-plan-correction state for the target could not be + /// read, or was read and held nothing. For the bot that is a blocker, not a note: see + /// . + public const string ReasonStateUnavailable = "state_unavailable"; + + /// FORCE_LAST_GOOD_PLAN is ON for the target's database — the engine's own bot is the + /// forcer there, and this one stands down for the whole database. See . + public const string ReasonApcEnabledForDatabase = "apc_enabled_for_database"; + + /// + /// The bot's whole blocker list for one target (#3654): the shared gate's verdict, BOTH halves + /// ( — the two + /// target-carried blockers and the five #3652 added from the store's forcing and automatic-plan- + /// correction state), then the two only an unattended actor needs. Pure: the caller reads the state + /// (DarlingForcePlanTargetStateReader, one batched statement per bot pass) and passes what it + /// got; every arm is pinnable without a store. + /// + /// Why the bot reads the whole gate and not the one-argument overload. #3652's live + /// cross-reference found five eligible: true, blockers: [] targets that were five-for-five + /// contraindicated by facts the store already held — two mid-verification under APC on exactly the + /// proposed plan, one a known forcing failure, one withdrawn by the engine, one already resolved + /// through another plan. The advisory surface learned to read them; a bot that still consulted the + /// state-less overload would have judged all five unblocked and, with every gate open, forced them. + /// Same function, same evidence strings — what an agent reads in structured_remediation is what + /// the bot enforces (#2146), and the journal row carries the evidence so a blocked decision is + /// auditable against the snapshot it was made on. + /// + /// apc_enabled_for_database — the engine is the forcer here. When + /// force_last_good_plan_actual_state is ON for the target's database, automatic plan + /// correction forces regressed queries' last good plans on its own, verifies them, and reverts the + /// ones that do not pay. A second forcer on the same database is exactly the failure #3652 + /// documented: a manual force on a plan APC is verifying converts AUTO to MANUAL and + /// deletes the engine's revert path, and a plan APC has not touched yet may be the one it is about to. + /// So the bot does not compete for the database at all — every target in it is blocked with this + /// name and the enablement snapshot as evidence, whatever the rest of the gate says. The advisory + /// surface, by contrast, only changes its VERB there (apc_mode: on plus guidance) and leaves + /// force_sql for the operator who has read it; a human can decide to intervene in an APC + /// database on purpose, and the bot must not. A database-level fact evaluated per target rather than + /// once per pass so the journal names it on every target it stopped and the cooldown dedups the + /// repeats, the same way the other blockers are recorded. + /// + /// state_unavailable — unknown fails closed. Two shapes, one blocker, evidence + /// distinguishing them. A null state (the read failed, or returned no row for this key) is the plain + /// case: the bot has no idea what the engine is doing. An EMPTY state (the read ran and observed + /// nothing inside ) is the subtler one: no + /// query_store_stats row for the plan is ordinary for a best plan that is not executing, and + /// no recommendation is ordinary for a query APC has not judged — but the enablement half comes from + /// a row PlanCorrectionCollector writes for EVERY database it enumerates, recommendation or not, + /// at the server's newest capture with no lookback bound. All four halves absent means the store + /// cannot see this database's FORCE_LAST_GOOD_PLAN state, so the arm above cannot be evaluated, and + /// an unattended forcer on a database whose APC enablement is unknown is the one place "unknown" + /// must mean "no". The advisory surface says state_note: unknown for both shapes and lets the + /// reader cross-reference (get_plan_corrections, sys.query_store_plan); the bot's + /// journal row says state_unavailable with the reason and forces nothing. The cost is a bot + /// that stays its hand on a target the store has not observed in a day — a best plan nobody has run + /// for 24 hours is thin evidence for an unattended force anyway. + /// + /// Order: the shared gate's blockers first (the names an agent already sees), then the bot's + /// own. A target can carry several — apc_owns_it and apc_enabled_for_database together + /// is the expected shape for a plan APC is verifying on an APC database — and the journal keeps all + /// of them; the decision is the same whichever fired first. + /// + /// What the store knows about this target now, or null when the read failed or + /// returned nothing for it. + /// The reader's stated reason when the whole read failed; quoted + /// into the state_unavailable evidence so the journal says WHY the bot could not see. + public static IReadOnlyList Blockers( + ForcePlanTarget target, + ForcePlanTargetState? state, + string? stateUnavailableReason) + { + if (target is null) + { + throw new ArgumentNullException(nameof(target)); + } + + var blockers = new List(FactRemediation.ForcePlanBlockers(target, state)); + + if (state is { ApcIsOn: true }) + { + blockers.Add(new ForcePlanBlocker( + ReasonApcEnabledForDatabase, + $"plan_correction: force_last_good_plan_actual_state = {state.ForceLastGoodPlanActualState} for {target.Database} at {FactRemediation.Stamp(state.EnablementObservedAtUtc)} — automatic plan correction owns plan forcing on this database; the bot stands down rather than be the second forcer (#3652: a manual force on a plan the engine is verifying replaces AUTO forcing and removes its revert path)")); + } + + if (state is null) + { + blockers.Add(new ForcePlanBlocker( + ReasonStateUnavailable, + string.IsNullOrWhiteSpace(stateUnavailableReason) + ? $"the forcing and automatic-plan-correction state read returned no row for plan {target.PlanId} of query {target.QueryId} in {target.Database}; an unattended force cannot proceed on an unknown engine state" + : $"{stateUnavailableReason.Trim()} — an unattended force cannot proceed on an unknown engine state")); + } + else if (state.IsEmpty) + { + blockers.Add(new ForcePlanBlocker( + ReasonStateUnavailable, + $"the forcing and automatic-plan-correction state read ran and observed nothing for this target inside the last {ForcePlanTargetState.Lookback.TotalHours:0} hours: no query_store_stats row for plan {target.PlanId}, no forced sibling plan of query {target.QueryId}, no plan_correction recommendation, and no plan_correction capture for {target.Database} at all — FORCE_LAST_GOOD_PLAN enablement is unknown for this database, and an unattended force cannot proceed on unknown")); + } + + return blockers; + } + + /// The blocker names, in order, for 's policyBlockers — and + /// the journal's reasons column. The evidence travels separately () so + /// the names stay the comma-joinable consumer API they have always been. + public static IReadOnlyList Names(IReadOnlyList blockers) => + blockers is { Count: > 0 } ? blockers.Select(b => b.Name).ToList() : Array.Empty(); + + /// The journal's detail for a blocked decision: one line per blocker, + /// name: evidence, so the row can be read against the snapshot it was judged on without a + /// second query. Null when there is nothing to quote. + public static string? Evidence(IReadOnlyList blockers) => + blockers is { Count: > 0 } + ? string.Join("\n", blockers.Select(b => $"{b.Name}: {b.Evidence}")) + : null; + public static ForcePlanBotDecision Evaluate( ForcePlanTarget target, IReadOnlyList policyBlockers,