Repository navigation
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
Merged
Conversation
…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.
erikdarlingdata
enabled auto-merge (squash)
September 18, 2026 23:29
erikdarlingdata
disabled auto-merge
September 18, 2026 23:43
This was referenced Sep 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The lie
PlanForceBot— the AUTOMATIC force-plan executor, #2138 phase 1 — judged every target throughFactRemediation.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:
DarlingForcePlanTargetStateReaderreturns oneForcePlanTargetStateper requestedForcePlanTargetKey— the target plan's newestquery_store_statsrow, the newest forced sibling plan, the newestplan_correctionrecommendation, and the database'sforce_last_good_plan_actual_stateat 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.
EvaluateTargetsAsyncnow materializes its candidate list first (CollectCandidates: PLAN_REGRESSION targets in finding order, one per (database, query), cut atMaxTargetsPerPass— 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 isIPlanForceActionStore.TryGetTargetStatesAsync, implemented byPgPlanForceActionStoreoverDarlingForcePlanTargetStateReader.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 ownServiceCommandDeadlines.PostAnalysisForcePlanSeconds(the reader gained an optionalcommandTimeoutSeconds; MCP callers still getMcpCommandDeadlines.ReadSeconds). On this seam rather than a new constructor parameter because the bot's every store read must be fakeable from one object, andDarlingWorkerkeeps constructing the bot with the journal and nothing else. A store fault returns as a reason, never throws; the pass-budget arithmetic inStragglerCommandTimeoutTestsnow 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 — isFactRemediation.ForcePlanBlockers(target, state)verbatim (same names, same evidence strings — what an agent reads instructured_remediationis what the bot enforces, #2146), then:apc_enabled_for_database—force_last_good_plan_actual_state = ONfor 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 leavesforce_sqlfor 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: noquery_store_statsrow 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 rowPlanCorrectionCollectorwrites 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: unknownand leaveseligibleas the finding-only verdict because its reader is a human or agent who will cross-referenceget_plan_correctionsandsys.query_store_planbefore 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'scollection_time, so ablockedrow is auditable against the state it was judged on. The two #2138 blockers now carry evidence the same way. Ablockedfrom 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 (
reasonsvalues ablockedrow 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), thenapc_enabled_for_database,state_unavailable(the bot's own,ForcePlanBotPolicy.Reason*— deliberately NOT in the shared vocabulary; an agent readingstructured_remediationmust never see them, because for a reader neither is a blocker; pinned). Plus the pre-existingfailed_force_cooldown,server_daily_budget_exhausted.What it does NOT do
grep PlanForceBot|ForcePlanBotPolicy Lite/→ nothing; the policy lives in Analysis and is shared, but no Lite caller exists to change.reasonsanddetailare unboundedtext), no write path, no executor —PlanForceNoWritePathTestsruns green against the freshly built Service assembly.DarlingWorkeruntouched: the bot's constructor is unchanged.would_forcerow'sdetailstays 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.ForcePlanBlockersoverload 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 andEveryGateOpen_…Withheldis 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 inreasons+ evidence fragment + snapshot stamp indetail(Theory); read failure →state_unavailablequoting the reader's reason; missing key →state_unavailablenaming the key; empty state →state_unavailablenaming the missing enablement; FLGP ON alone →apc_enabled_for_databasewith the enablement evidence; FLGP ON + APC verifying →apc_owns_it,apc_enabled_for_databaseboth 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):Blockersis 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;Evidenceformat; the bot's two reasons are absent fromForcePlanBlockerNames;Evaluatewith 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 saysapc_owns_it, the bot's list equals the shared gate's, FLGP ON adds the stand-down where the projection saysapc_mode: on, a failed read is a blocker where the projection saysstate_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 + 3PlanForceNoWritePathTestsover 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 isStateReaderSql_IsTheDarlingText…'sParitySource.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=trueat 0 warnings from this change (Lite.Tests carries one pre-existing xUnit2000 inLogTailOverlapThresholdPinTests.csfrom #3646, not touched here).Closes #3654