Add auto force-plan bot phase 2: the write path (#2138) - #2731
Add auto force-plan bot phase 2: the write path (#2138)#2731erikdarlingdata wants to merge 7 commits into
Conversation
…2138) Pure and clockless, in the shared Analysis assembly: ForcePlanBotPolicy consumes FactRemediation.ForcePlanBlockers' own output (the #2146 contract - one gate for agents and bot alike), layers the bot's gates on top (regression floor, per-query cooldown, windowed failed-force memory, per-server daily budget), and only returns Force when BOTH write gates - global dry-run off AND the per-server opt-in - are open. Defaults are globally OFF, then DRY RUN. ForcePlanSelfReview re-judges a live force at +1h/+24h against the journaled baseline and unforces when it is not at least 25% better or fails to apply. Give-up state is a sliding window the caller's read computes, never a flag (#2677). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JgLaaxFenZFZs1SntKQDEg
…2138) collect.plan_force_actions is the bot's append-only audit trail - every would-force, blocked, force, review, and unforce row with its evidence, outcomes as their own rows via related_action_id, deliberately outside retention. config_monitored_servers.plan_force_bot_enabled is write-gate 2, NOT NULL DEFAULT FALSE on every existing and future row. Viewer probe: table-existence sentinel + newest-first arm, and the reflection-built all-true pin tracks the new parameter by arity. Ladder applied cleanly on a live PostgreSQL 17, rung re-run proven idempotent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JgLaaxFenZFZs1SntKQDEg
…2138) The bot rides the SCHEDULED analysis pass only (analyze_now passes a null hook so an operator poking a server cannot spend the blast-radius budget), re-judges the pass's PLAN_REGRESSION targets through the shared blocker gate, journals every decision, and reviews its own outstanding live forces first - own-forces-only structurally, because the review read starts from rows this bot journaled. SqlServerPlanForceExecutor is the ONLY route to a monitored server: the #1914-verified named two-argument force/unforce, a verify read that grades the WHOLE query since the force (weighted vs the journaled baseline), and the targeted DBCC FREEPROCCACHE(plan_handle) evict lever (phase 2 orchestration). Values travel as bound parameters; the database as ChangeDatabase. Config: forcePlanBot section (file-level, OFF then DRY RUN) documented in the sample; the per-server opt-in column is store-only on purpose - the registry is authoritative after seeding, so a darling.json knob would be a silent no-op (#2254), which is disqualifying for a write authorization. Seed INSERT and registry SELECT alignment proven against a live V106 store (opt-in seeds FALSE; is_enabled TRUE). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JgLaaxFenZFZs1SntKQDEg
#2138) The two contracts that must never regress, as tests: a blocked target - parameter sensitivity above all - can never evaluate Force or WouldForce whatever the gates say, and a live Force requires BOTH write gates with each closed gate NAMED in the journaled reasons. Failed-force memory is pinned healing by the window sliding, with no reset call in the API. The executor pins are character-exact - the argument forms #1914 measured, the divide guard, the bounded evict lookup, and the absence of the argumentless FREEPROCCACHE that would clear a whole cache. The store round-trip (gated on DARLING_TEST_PG) proves the windowed history reads, the intent/completion budget single-count, the pending- review linkage, and the fleet-wide null-server read against a real store. Plus the CHANGELOG entry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JgLaaxFenZFZs1SntKQDEg
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| the DBNull guard is for a store mid-migration rather than an expected path — and a null there | ||
| falls back to the derivation, which is exactly what it did before this column was read at all. */ | ||
| StoredServerId = reader.IsDBNull(15) ? null : reader.GetInt32(15), | ||
| /* V106 (#2138): write-gate 2 for the force-plan bot. NOT NULL DEFAULT FALSE in the table, so the |
There was a problem hiding this comment.
Comment says "V106" but plan_force_bot_enabled is actually added by migration V107 (PgMigrations.cs: new Migration(107, "plan-force-actions", V107Sql), and StorageVersion.SchemaVersion = 107). V106 is the unrelated pg-cpu-utilization rung. Given this codebase's stated convention that comments carry accurate provenance (issue numbers, measurements, version numbers) for future readers, this stale/incorrect version reference should be fixed to V107.
| { | ||
| /* Nothing cached under that hash is a legitimate outcome (the plan aged out on its | ||
| own), not a failure — the journal records it and the caller decides what it means. */ | ||
| return PlanForceExecutionResult.Failed("no cached plans matched the hash — nothing to evict"); |
There was a problem hiding this comment.
This contradicts the comment immediately above it (lines 165-168): "Nothing cached under that hash is a legitimate outcome (the plan aged out on its own), not a failure — the journal records it and the caller decides what it means." But the code returns PlanForceExecutionResult.Failed(...), which PlanForceBot's eventual phase-2 caller would journal with Outcome = OutcomeFailed. That outcome string is exactly what GetQueryHistoryAsync's recent_failed aggregate counts (pfa.action = 'force' AND pfa.outcome = 'failed'), so once phase 2 wires EvictPlanAsync into the bot's flow, a query whose bad plan simply aged out of cache — a good outcome — would count toward FailedForceThreshold and could cool the query down for FailedForceCooldownHours. This method is unwired/dead code today (no caller in PlanForceBot.cs), so it's latent rather than live, but worth fixing before phase 2 lands so it doesn't ship with an incorrect classification baked into the executor's contract.
| /// <para>Failure-isolated at every seam: a journal or executor fault logs and moves on — the bot | ||
| /// must never take an analysis pass or a sweep down with it.</para> | ||
| /// </summary> | ||
| public sealed class PlanForceBot |
There was a problem hiding this comment.
Test-coverage gap: ForcePlanBotPolicy, ForcePlanSelfReview, PgPlanForceActionStore, and SqlServerPlanForceExecutor all have dedicated test files, but PlanForceBot itself — the orchestrator that wires them together — has none (Darling.Tests/ has no PlanForceBotTests.cs). This class carries logic that isn't exercised elsewhere, e.g.:
EvaluateTargetsAsync'sMaxTargetsPerPasscap and theseendedup-across-findings set,- the intent-row/completion-row sequencing and
relatedActionIdwiring inExecuteForceAsync, - and, most importantly, the write-gate re-check in
ExecuteUnforceAsync(gatesOpen = !_settings.DryRun && currentConfig.PlanForceBotEnabled) that decides whether a self-review'sUnforceverdict actually reaches the server or gets journaled as withheld.
Given this is explicitly called out as "the FIRST feature that can WRITE to a monitored SQL Server," a fake IPlanForceExecutor/PgPlanForceActionStore-backed test of this orchestration (particularly the withheld-vs-executed unforce branch) seems worth having before phase 2 raises the stakes further.
|
Reviewed the phase-1 force-plan bot (policy, self-review, journal store, executor, worker wiring, and migrations). This is a large, carefully-guarded change and the core decision logic is thoroughly pinned by tests ( Left 3 inline comments:
No SQL injection concerns ( |
The four registration points the recipe grep missed, each caught by its own ratchet: the probe reader now consumes ordinal 82 (the five three-places-agree pins); PlanForceBotEnabled joins the round-trip catalog; plan_force_bot_enabled is classified NON-SECRET in DarlingManagedRoles (an arm/disarm state as sensitive as is_enabled - the viewer must show which servers are armed for the opt-in to be auditable); and the store test's teardown goes through LiveStoreCleanup (#1902) instead of hand-opened connections. Review findings: the stale V106 comment in BuildServerFromRow says V107; an evict that finds nothing cached returns NoOp, not Failed - the plan aging out is the GOAL state, and a failed outcome would have fed the failure-memory window and cooled a query down for succeeding once phase 2 wires eviction in. The coverage catch: PlanForceBot itself now has a spec. IPlanForceActionStore is extracted so the orchestrator runs over an in-memory journal and a recording executor, and PlanForceBotTests pins the seams no other suite can see - the engine gate at the connection boundary, cross-finding dedup + the per-pass cap, intent-then-completion linkage, and the write-gate RE-CHECK on the unforce path (a self-review verdict journals as withheld, executor untouched, when either gate closed after the force). All eleven scenarios also executed green on macOS against the real assemblies via the net10.0 harness. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JgLaaxFenZFZs1SntKQDEg
| if (!_settings.Enabled || runtime is null || currentConfig is null) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Flipping the global Enabled gate off doesn't just stop new evaluations — it also skips ReviewPendingForcesAsync entirely, since that call sits behind this same early return and only runs from RunAfterAnalysisAsync. That means any plan the bot already force-pinned live stays forced with no further self-review once an operator sets forcePlanBot.enabled: false (and restarts, since this is file-level and not hot-reloaded).
Contrast this with the other two gates, which both keep reviewing on purpose:
DryRun = truestill runsReviewPendingForcesAsyncand journals a "withheld" unforce with a by-handEXEC sys.sp_query_store_unforce_planhint (ExecuteUnforceAsync, line ~304-322).- Per-server
PlanForceBotEnabled = falsehits the same withheld path.
So the one lever an operator is most likely to reach for after discovering a bad force — "turn the whole bot off" — is the one lever that silently disables the safety net (self-review / auto-unforce) for a plan it already pinned on a production server, with nothing journaled or logged to say review has stopped. Given how carefully this PR treats every other "gate closed after a force" case (always journaling a withheld verdict with a manual-fix hint), this asymmetry looks like an oversight rather than intent.
Worth considering: let ReviewPendingForcesAsync run (or at least the read-only VerifyAsync + a withheld-unforce journal entry) even when _settings.Enabled is false, or explicitly document that enabled: false abandons outstanding live forces and operators must use dryRun: true instead to keep the safety net active.
|
Reviewed the diff (policy, self-review, executor, orchestrator, journal store, migration, config/registry wiring, tests) against CONTRIBUTING's T-SQL/PostgreSQL conventions, Lite/Darling parity, and security/correctness at the write boundary. Overall: this is unusually careful work for a first write path to production SQL Servers. The two-gate contract (global One correctness/safety gap (posted inline on Lite/Darling parity: no drift — this is explicitly a Darling-only feature (phase 3 in the PR description defers "Lite twin, if ever" as a discussion item), and the two new files in the shared Style: collector-style queries ( Nothing else rose to the level of a blocking finding — the test suites ( |
….sql (#2138) The #1639 drift ratchet working as designed: the operator-facing provisioning script's GRANT SELECT column carve is a hand mirror of DarlingManagedRoles.ViewerRestrictedConfigTables, and ProvisionRolesAclDriftTests parses the script and asserts set-equality - so classifying the column in C# alone failed both CI jobs until the script named it too. Non-secret, exactly like is_enabled beside it. Comment kept parenthesis-free inside the GRANT list on purpose: the parser strips -- comments before matching, but the paren-scan that finds the list's end does not need the extra test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JgLaaxFenZFZs1SntKQDEg
| var executor = _executorFactory(runtime.ConnectionString); | ||
| var result = await executor.ForcePlanAsync(target.Database, target.QueryId, target.PlanId, ct); | ||
|
|
||
| await _store.JournalAsync(BuildRecord( | ||
| runtime, target, PgPlanForceActionStore.ActionForce, Array.Empty<string>(), | ||
| result.Succeeded ? PgPlanForceActionStore.OutcomeSucceeded : PgPlanForceActionStore.OutcomeFailed, | ||
| detail: result.Error, relatedActionId: intentId, DateTime.UtcNow), ct); |
There was a problem hiding this comment.
Correctness: a force that succeeds on the server can become permanently un-reviewable if the completion journal write fails.
ExecuteForceAsync writes the intent row, calls executor.ForcePlanAsync (which really executes sp_query_store_force_plan on the monitored server), and then writes the completion row. If ForcePlanAsync succeeds but this second JournalAsync call throws (e.g. a transient Postgres blip unrelated to the SQL Server connection that just succeeded), the exception propagates out of EvaluateTargetsAsync and is swallowed by the outer try/catch in RunAfterAnalysisAsync (line 98-107) — logged as a warning and nothing else.
The consequence: the plan is forced on the monitored server, but no row with outcome = 'succeeded' and action = 'force' ever lands in collect.plan_force_actions. GetPendingReviewsAsync (PgPlanForceActionStore.cs) only surfaces rows matching exactly that shape, so this force can never appear as "pending review" — ReviewPendingForcesAsync will never call VerifyAsync/ForcePlanSelfReview.Evaluate for it, and the self-review "pays rent or gets taken back" guarantee that's the centerpiece of this PR's safety story silently never applies to it. It also isn't retried: nothing re-attempts the completion journal write on a later pass.
Compare with the analogous unforce path (ExecuteUnforceAsync): if its completion write fails after a successful UnforcePlanAsync, the row still self-heals next pass, because the pending-review row from the original force is still there, and the next VerifyAsync will observe PlanIsStillForced == false and close it out via a review row. The force path has no equivalent fallback because the "pending" status is established by the very completion write that's at risk.
Worth considering: write the completion row in a retry loop (or at least a couple of attempts) before giving up, or have a periodic reconciliation pass that lists forced plans on the server (or a way to detect intent rows stuck in attempting past some age) and treats them as pending review. As shipped, this is a narrow race window, but it directly undermines the one invariant ("a force that buys nothing still pins a plan against future data change, so it has to pay rent") that the rest of the design is built around.
|
Reviewed the diff (2,773 insertions across 21 files: Overall: this is unusually well-defended code for a first write path. The SQL surface is fully parameterized (no string concatenation anywhere — database name travels via One correctness finding posted inline on Lite/Darling parity: no Lite changes in this PR, and none are needed — the PR description explicitly scopes this to Darling only (phase 3, "Lite twin, if an interactive SKU should ever act autonomously at all," is left as a future discussion item), and the journal's Security: no SQL injection surface — every value is a bound parameter on both the SQL Server and Postgres sides. The write path is a single reviewable seam ( Performance: bounded per-pass ( Nothing else rose to the level of a blocking finding — the extensive test suites ( |
Two categorical gaps in the review loop, both real: 1. Disarming the bot orphaned outstanding live forces: the whole pass sat behind the Enabled gate, so forcePlanBot.enabled: false left a bot-pinned plan forced forever with nobody watching. The review half now deliberately outlives the switch that armed the force - a disabled bot still verifies its OWN outstanding forces (one indexed read of a journal that is empty unless this deployment ever forced live), journals the verdict, and WITHHOLDS the write (gatesOpen now includes Enabled) - one actionable row + log line per orphaned force, then quiet. Only the evaluation half is Enabled-gated. 2. A force that landed on the server but lost its completion journal write (store blip between the two rows) became permanently un-reviewable: pending reviews keyed on succeeded completion rows only. The read now also surfaces ORPHANED INTENTS - attempting rows past a 10-minute grace that no completion row references - and the verify read answers what actually happened: still forced gets a real review, never landed closes as no_longer_forced. Both pinned in PlanForceBotTests and the live-store round-trip, and both executed green on macOS against the real assemblies and a live PostgreSQL 17. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JgLaaxFenZFZs1SntKQDEg
erikdarlingdata
changed the title
|
Claude posting for Erik Darling Closing: divergent second implementation of a phase that already landed, 672 commits behind Phase 1 reached #2138 stays open, because the work this branch aimed at is genuinely outstanding and deliberately fenced. So phase 2 should be rewritten against Closing by hand; closing keywords do not fire here. |
…rikdarlingdata#2138) The orchestrator runs after each SCHEDULED analysis pass (analyze_now passes a null hook — an operator poking a server should not spend the bot's blast-radius budget), judges the pass's PLAN_REGRESSION targets through the shared gate, and journals the verdicts. It cannot write to a monitored server, and that is asserted three ways rather than promised. IPlanForceExecutor is declared with NO implementation; PlanForceBot holds no executor, no connection factory and no member that names the seam; and the compiled service assembly is searched byte-wise for sp_query_store_force_plan, sp_query_store_unforce_plan and FREEPROCCACHE and must contain none. Proven red by dropping the write path back in: all three go red, and they go red again when erikdarlingdata#2731 lands it — which is the point. Relaxing them is the reviewable act, not a side effect. Open all three gates on this build and the Force verdict journals as withheld, its own outcome: not 'failed' (nothing failed, and a failed force would cool the query down for a week) and not 'attempting' (which would surface as an orphaned intent owed a review of a force that never happened). Saying so out loud beats a silent downgrade to would-force — an operator who opened every switch believes the bot is live. Engine-gated at the boundary a connection would cross, not at the upstream fact (erikdarlingdata#2213's lesson), and failure-isolated at both seams so a bot fault can never reclassify a good analysis pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JgLaaxFenZFZs1SntKQDEg
1 participant
Draft, per your call that this is too much for a single pass. The honest headline first: the live write path IS in this diff —
sp_query_store_force_plan/_unforce_plan/ targetedDBCC FREEPROCCACHE(plan_handle)all exist inPlanForceExecutor.cs, complete and pinned, reachable only behind two gates that both ship closed.How to review this (the risk lives in 5 files, in this order)
PerformanceMonitor.Analysis/ForcePlanBotPolicy.cs(256 LOC) — the decision table. (Landed in Add force-plan bot phase 1: detection, evidence journal, dry-run (#2138) #2745.) Invariants pinned byForcePlanBotPolicyTests: shipped defaults are OFF + DRY RUN; a target with anyForcePlanBlockersoutput (PSP, secondary-replica evidence) can never evaluateForceorWouldForcewhatever the gates say — the #2138 gap 3: parameter-sensitivity co-fire flag + force-plan caution #2140 never-auto-force rule as a testable contract, same function agents read instructured_remediation(#2138: machine-first structured_remediation for MCP consumers #2146); a liveForcerequires dry-run off AND per-server opt-in, each closed gate named in the journaled reasons; failure memory is a sliding window with no reset API ([BUG] 3.5.0->3.6.0 Deadlocks #2677).Darling/.../PlanForceExecutor.cs(286 LOC) — the whole subject of this PR after the rebase. The ONLY statements that can ever reach a monitored server.PlanForceExecutorTestspins them character-exact: the Collect replica_group_id so a force-plan recommendation can be replica-scoped, not just replica-labelled #1914-verified named two-argument force form (no@replica_group_id, no@disable_optimized_plan_forcing), noUSE/CONVERTanywhere, evict is the bounded handle form — the argumentless cache-wideFREEPROCCACHEis pinned ABSENT, and a garbage plan-hash refuses to parse rather than evicting by accident.Darling/.../PlanForceBot.cs— the orchestration, where a write would escape. The evaluate-and-journal half landed in Add force-plan bot phase 1: detection, evidence journal, dry-run (#2138) #2745; the force and review-unforce arms stay here.PlanForceBotTests' write-path scenarios pin: intent-row-then-completion-row journaling linked byrelated_action_id; and the write-gate RE-CHECK on unforce — a self-review verdict journals as withheld, executor untouched, when ANY gate (including the global Enabled) closed after the force. The review half deliberately outlives the Enabled switch: disarming the bot cannot orphan a force it placed while armed.Darling/.../PgPlanForceActionStore.cs(landed in Add force-plan bot phase 1: detection, evidence journal, dry-run (#2138) #2745) — the windowed history reads ARE the cooldowns.PlanForceActionStoreTests(live PG, CI-gated) pins: an intent+completion force pair spends ONE budget slot; failure memory heals purely by the window sliding; a pending review is closed only by a related review/unforce row; an ORPHANED INTENT (a force that landed but lost its completion journal write) surfaces for review after a 10-minute grace, so no live force can escape review through a store blip; the fleet-wide null-server read works.PerformanceMonitor.Analysis/ForcePlanSelfReview.cs(landed in Add force-plan bot phase 1: detection, evidence journal, dry-run (#2138) #2745) — the review verdict table;ForcePlanSelfReviewTestsis its complete spec (not-net-benefit unforces at the FIRST due checkpoint; exactly-at-the-bar keeps; a quiet query keeps its force with the reason saying the keep rests on absence of evidence; "no longer forced" outranks "failing to force").Phased plan (what's here vs. what follows)
get_plan_force_actions(web-parity pin touchesDarlingWebEndpoints.cs, owned by the #2710 lane right now)Numbers (pre-carve)
21 files, 2,866 insertions / 12 deletions. Production code ~1,600 LOC across 8 files; tests ~1,430 LOC across 6. Verified: full ladder V1→V107 + journal round-trip green on live PostgreSQL 17; all 50 policy/review/orchestrator scenarios executed green on macOS against the real assemblies (the Windows suites are CI's). Renumbered V106→V107 mid-flight after #2719 merged claiming V106; #2745 re-derived the same 107 as
max(dev) + 1and keeps it. CI rounds 1-2 caught five registration ratchets (probe reader ordinal, round-trip catalog, role ACL + its provision-script mirror, the #1902 cleanup pattern) and the review bot five real findings across three rounds (stale V107 comment; evict no-op misclassified as failure; missing orchestrator spec; disarming the bot orphaned outstanding forces from self-review; a force that lost its completion journal write became un-reviewable) - all addressed in follow-up commits, no force-pushes.🤖 Generated with Claude Code
https://claude.ai/code/session_01JgLaaxFenZFZs1SntKQDEg