Skip to content

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, fails closed on an unknown state, stands down where FORCE_LAST_GOOD_PLAN is on, and journals every blocker with its evidence (#3654) - #3663

Merged
erikdarlingdata merged 1 commit into
devfrom
fix/3654-lane
Sep 18, 2026

Conversation

@erikdarlingdata

Copy link
Copy Markdown
Owner

The lie

PlanForceBot — the AUTOMATIC force-plan executor, #2138 phase 1 — judged every target through FactRemediation.ForcePlanBlockers(target), the one-argument overload #3652 kept for compatibility. That overload reads two facts the persisted target carries (PSP co-fire, non-primary replica) and nothing else. So the bot's gate had exactly the gap #3652 closed on the advisory surface: handed the five live targets that lane cross-referenced — two mid-verification under automatic plan correction on exactly the proposed plan, one a known forcing failure on that plan, one withdrawn by the engine, one already resolved through a different plan — it would have judged all five unblocked and, with every gate open, forced them. The bot is dry-run-gated with no write path today (write-gate 2, V107; PlanForceNoWritePathTests), which is why this is a follow-up rather than part of #3652; it must not go live without it.

Mechanism

The store already knew everything and #3655 already wrote the reader: DarlingForcePlanTargetStateReader returns one ForcePlanTargetState per requested ForcePlanTargetKey — the target plan's newest query_store_stats row, the newest forced sibling plan, the newest plan_correction recommendation, and the database's force_last_good_plan_actual_state at the server's newest capture. The MCP tools call it once per result and pass the states to the two-argument gate. The bot never did.

The fix

One batched read per pass, through the store seam. EvaluateTargetsAsync now materializes its candidate list first (CollectCandidates: PLAN_REGRESSION targets in finding order, one per (database, query), cut at MaxTargetsPerPass — the same order and cap the walk produced, so the worst-regression-first assumption the cap rests on is unchanged), reads every candidate's state in ONE statement, then judges each. The read is IPlanForceActionStore.TryGetTargetStatesAsync, implemented by PgPlanForceActionStore over DarlingForcePlanTargetStateReader.ReadAsync — the SAME statement the MCP surfaces run, so the bot's state half and the advice's cannot disagree from the same store — under the bot's own ServiceCommandDeadlines.PostAnalysisForcePlanSeconds (the reader gained an optional commandTimeoutSeconds; MCP callers still get McpCommandDeadlines.ReadSeconds). On this seam rather than a new constructor parameter because the bot's every store read must be fakeable from one object, and DarlingWorker keeps constructing the bot with the journal and nothing else. A store fault returns as a reason, never throws; the pass-budget arithmetic in StragglerCommandTimeoutTests now counts it: (2 × 10 + 1) × 5 s = 105 s ≤ 120 s.

The bot's gate is the whole gate plus two of its own. ForcePlanBotPolicy.Blockers(target, state, stateUnavailableReason) — pure, on the policy, pinnable without a store — is FactRemediation.ForcePlanBlockers(target, state) verbatim (same names, same evidence strings — what an agent reads in structured_remediation is what the bot enforces, #2146), then:

  • apc_enabled_for_database — force_last_good_plan_actual_state = ON for the target's database. The engine's own bot is the forcer there; a second forcer on one database is the failure Force-plan remediation reports 'eligible, no blockers' for plans automatic plan correction already owns, has already failed to force, or has withdrawn — five for five on one incident #3652 documented (a manual force on a plan APC is verifying converts AUTO→MANUAL and deletes the revert path, and a plan APC has not touched yet may be the one it is about to). The bot stands down for the whole database — 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 only changes its VERB there (apc_mode: on + guidance) and leaves force_sql for an operator who has read it; the bot cannot be the operator who read the guidance.
  • state_unavailable — unknown fails CLOSED. Two shapes, one name, evidence distinguishing them: a null state (the read failed — reader's reason quoted — or returned no row for the key), and an EMPTY state (the read ran and observed nothing inside the 24 h lookback on any half). The second is the subtle one and I want it reviewed: no query_store_stats row for the plan is ordinary for a best plan that is not executing, and no recommendation is ordinary — 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 FLGP 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 cost is a bot that stays its hand on a target the store has not observed in a day — thin evidence for an unattended force anyway. A plan not observed but a database that IS (enablement OFF, everything else null) is NOT unavailable and the bot proceeds (pinned).

Why the bot fails closed where the advice merely notes — the comment at the call site says it in full: the advisory surface writes state_note: unknown and leaves eligible as the finding-only verdict because its reader is a human or agent who will cross-reference get_plan_corrections and sys.query_store_plan before running anything (#3652 was found exactly that way). The bot has no reader between its verdict and the write. And every one of the five #3652 blockers fires only on an OBSERVED value, so an unread state does not weaken the gate — it reproduces the pre-#3652 gate exactly.

The journal carries the evidence. reasons = the blocker NAMES, comma-joined as ever (the consumer API). detail = name: evidence, one per line (ForcePlanBotPolicy.Evidence), stamped with each snapshot's collection_time, so a blocked row is auditable against the state it was judged on. The two #2138 blockers now carry evidence the same way. A blocked from the bot's own history gates (failed_force_cooldown, server_daily_budget_exhausted) keeps a null detail as before. The policy's ordering did not move: the cooldown still outranks the blockers, whichever half they came from, so an APC-owned target journals once per window, not every pass.

Journal vocabulary as landed (reasons values a blocked row can carry): parameter_sensitivity_cofired, secondary_replica_evidence, apc_owns_it, already_forced, forcing_failed_on_this_plan, apc_withdrew_it, apc_resolved_differently (the shared gate's, ForcePlanBlockerNames), then apc_enabled_for_database, state_unavailable (the bot's own, ForcePlanBotPolicy.Reason* — deliberately NOT in the shared vocabulary; an agent reading structured_remediation must never see them, because for a reader neither is a blocker; pinned). Plus the pre-existing failed_force_cooldown, server_daily_budget_exhausted.

What it does NOT do

  • Lite has no bot. grep PlanForceBot|ForcePlanBotPolicy Lite/ → nothing; the policy lives in Analysis and is shared, but no Lite caller exists to change.
  • No schema change (reasons and detail are unbounded text), no write path, no executor — PlanForceNoWritePathTests runs green against the freshly built Service assembly.
  • DarlingWorker untouched: the bot's constructor is unchanged.
  • The would_force row's detail stays null; a row dated after this lands means the state was read and clean. Recording the clean snapshot on would-force rows would help the dogfood ledger and is a one-line follow-up if wanted.
  • The one-argument ForcePlanBlockers overload remains (tests pin the two halves agree on it); nothing that acts consults it any more, and its doc says so.

Tests

Darling.Tests/PlanForceBotTests.cs (+15 → 22): the fake store answers the state read (default: read, observed, clean — FLGP OFF, nothing forced, no recommendation — so the seven pre-#3654 tests keep their meaning and EveryGateOpen_…Withheld is now also the happy path of the state half). New: the state is read ONCE for the pass and asked about exactly the ten capped targets; no candidates → no read; each of the five live #3652 shapes stops the force with every gate open and journals name in reasons + evidence fragment + snapshot stamp in detail (Theory); read failure → state_unavailable quoting the reader's reason; missing key → state_unavailable naming the key; empty state → state_unavailable naming the missing enablement; FLGP ON alone → apc_enabled_for_database with the enablement evidence; FLGP ON + APC verifying → apc_owns_it,apc_enabled_for_database both with evidence, shared first; PSP still rides the same row with evidence now; a history-gate block keeps null detail; cooldown still outranks state blockers.

Darling.Tests/ForcePlanBotPolicyTests.cs (+16 → 30): Blockers is empty on a clean observed state; carries the shared gate verbatim; FLGP ON adds the stand-down after the shared gate, case-insensitive on ON/on/On, nothing on OFF/off; null state with/without reason; null state still carries the target half first; empty state is unavailable; plan-not-observed-but-database-is is NOT; Evidence format; the bot's two reasons are absent from ForcePlanBlockerNames; Evaluate with the bot's blockers is Blocked with every gate open.

Lite.Tests/ForcePlanTargetStateTests.cs: ThePlanForceBotsGate_IsTheOriginalOverload_… (the gap pin) → ThePlanForceBotsGate_IsTheWholeGate_PlusItsOwnTwo_… (the closure pin): on the live APC-verifying shape the one-argument overload says nothing, the shared gate says apc_owns_it, the bot's list equals the shared gate's, FLGP ON adds the stand-down where the projection says apc_mode: on, a failed read is a blocker where the projection says state_note: unknown, and the overloads still agree on the target half.

Darling.Tests/StragglerCommandTimeoutTests.cs: the pass-budget derivation counts the batched read.

Executed on this Mac: Darling arms via a throwaway net10.0 harness (AssemblyName Darling.Tests, ProjectReference the WPF-free Service project, Compile-Include the three files) — 55/55 green (22 bot + 30 policy + 3 PlanForceNoWritePathTests over the built Service dll). Lite pin via the lane-3652 harness pattern (net10.0-windows, WindowsDesktop framework entry removed) — 23/24 green; the one red is StateReaderSql_IsTheDarlingText…'s ParitySource.ReadFile, which walks up to the repo root and cannot from an out-of-tree bin (same as #3655's report); the SQL template is untouched by this PR and the exact comparison replayed in Python over the two source files reads equal, casts as written. No rig: the reader's SQL did not change. Analysis, Service, Darling.Tests and Lite.Tests build -c Release -p:EnableWindowsTargeting=true at 0 warnings from this change (Lite.Tests carries one pre-existing xUnit2000 in LogTailOverlapThresholdPinTests.cs from #3646, not touched here).

Closes #3654

…ould 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant