diff --git a/CHANGELOG.md b/CHANGELOG.md index fe4d12607f..e6bcf265da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added +- **The sweep-body detach policy (#2700/#2717) is now pinned, including the invariant that makes it safe** ([#2840]) - `query_store` and `plan_correction` are fired DETACHED from the sequential per-server collection body, because the outer launch loop will not relaunch that body while it runs (INV-2, one body per server), so one slow collector delays every other collector for that server. That set had no test at all, and the criterion for admitting a third collector was recorded nowhere. Measured against production `collection_log`, the set is exactly right: `query_store` at p90 65,053ms and `plan_correction` at max 133,934ms are the only per-cycle cost outliers, and the intuitive derivation from enumerated-vs-scalar fanout is disproved in BOTH directions - `database_scoped_config` fans out to 10 databases at p90 127ms, while `procedure_stats` has zero fanout at p90 11,964ms, so a fanout-derived split would detach the cheap collector and leave the expensive one starving the tier. The pin that generalizes asserts no detached collector sits on the 1-minute tier: a detached run SKIPS when its previous run is still in flight, so detaching a 1-minute collector converts starvation into guaranteed misses - exactly what would happen if the residual ~1.5-minute floor ([#2841]) were "fixed" by detaching `procedure_stats`. - **`query_store` plan and text fetch phases now name the STORE half separately from the TARGET half** ([#2811]) - `plan_fetch:Nms` and `text_fetch:Nms` each time a whole METHOD, not a query: opening a store connection, a touch/probe round trip against the store, at most two statements against the monitored server, and a store write. Three of those four steps are Postgres, and the blended number cannot say which held the time - so a 189,562ms `plan_fetch` measured on a production database (85% of a 224s run) was read as SQL Server query time for a full day. Two measurements of that target statement disagree by two orders of magnitude and BOTH are real: ~57,073ms CPU per execution against the collector's actual workload of cold plans missing from the store, versus ~650ms for 400 ids in a controlled A/B over hot recently-executed plans. Cycles that run a separate fetch now emit a second line - `plan_fetch:Nms = probe:Nms + target:Nms + write:Nms + other:Nms (N chunk(s), N ids)` - where `other:` is the computed residual so the parts SUM to the parent by construction rather than approximately, and a large `other:` is itself the finding (the cost would be in the collector's own bookkeeping, in neither database). The id and chunk counts make per-id cost computable, which is the only honest way to compare a cold production pass against a hot-plan benchmark. On its OWN line rather than nested in the existing one, because that line is parsed by tooling outside this repo and every existing collector line stays byte-identical. Same argument as [#2312] one seam further down, and instrumentation only - no query, hint or retention change. - **OIDC sign-in for the web dashboard: token validation and claim-to-role mapping** ([#2550]) - opt-in via `web.network.oidc` (authorization code + PKCE against any standard OIDC provider, resolved through its discovery document; the client secret takes the same DPAPI / `env:` / `file:` treatment as the endpoint tokens). The login page grows an SSO button, a successful sign-in mints the SAME `#2583` session cookie with the authenticated subject and role sealed inside the HMAC-signed subject slot, and the shared `?token=` path keeps working byte-for-byte unchanged as the scripted-caller / break-glass credential - OIDC entirely OFF when unconfigured, zero change for existing deployments. Role mapping: `roleClaim` + `adminRoles`/`viewerRoles` map IdP groups onto two seats - edit, or read-only - and a signed-in user matching neither list is refused outright, which is per-user revocation working. The read-only seat is enforced by a group-level middleware gate ahead of routing rather than per-endpoint attributes, so endpoints added by other lanes are covered automatically (verified live against `#2729`'s triage endpoint: GET 200, POST 403, with no wiring in that lane). ID tokens are validated for issuer, audience + `azp`, expiry with clock skew, and the nonce binding them to the transaction this host started; the discovery document's issuer must equal the configured authority and its endpoints must be `https` (or loopback), so a discovery response cannot choose the issuer or redirect the secret-bearing token POST to cleartext. Sign-in refusals ride the rate-limited refusal log; a subject carrying ASCII control characters is refused rather than laundered, so it can never forge a line in the audit trail. Exercised end-to-end against a live Keycloak with real users, groups and a confidential client: full code+PKCE handshake, admin and viewer mapping, unmapped-user refusal, wrong password, tampered cookie, forged state, shared-token fallback, and logout. - **The auto force-plan bot's detection half arrives, and it cannot write to anything** ([#2138] phase 1) - the bot re-judges each scheduled analysis pass's `PLAN_REGRESSION` force-plan targets through the SAME `FactRemediation.ForcePlanBlockers` gate agents already read in `structured_remediation` (#2146) - one policy, so advise and act cannot drift, and the #2140 never-auto-force-a-parameter-sensitive-target rule is now enforced code rather than prose. Every decision lands in schema **v107**'s `collect.plan_force_actions` journal with the evidence that produced it (regression factor, cpu/exec numbers, blockers, mode, outcome). **This phase ships no write path at all**: the `IPlanForceExecutor` seam is declared with no implementation, `PlanForceBot` holds no executor and no connection factory, and three tests pin it - the compiled service assembly is searched byte-wise for `sp_query_store_force_plan` / `sp_query_store_unforce_plan` / `FREEPROCCACHE` and must contain none, no shipped assembly may contain a concrete `IPlanForceExecutor`, and `PlanForceBot` may not name the seam in any member signature. So the whole feature is detection, evidence and a dry-run ledger; the write path is its own reviewable change. The journal is APPEND-ONLY (outcomes and reviews are their own rows via `related_action_id`, never UPDATEs) and deliberately not enrolled in retention - an audit of what a bot decided about production outlives the metrics that motivated it. The gates it will one day guard a write with are all here and all shipped closed: `forcePlanBot.enabled` false, `forcePlanBot.dryRun` true, and the new per-server `config_monitored_servers.plan_force_bot_enabled` opt-in, which defaults FALSE on every row and has NO darling.json counterpart on purpose (the registry is authoritative after seeding, so a file knob would be a silent no-op - the #2254 trap applied to a write authorization). Open all three on this build and the decision journals as WITHHELD, which is the honest answer rather than a silent downgrade to would-force. Flap-proofing is structural and rehearsed in shadow mode, because dry-run is not a separate code path: it spends the same per-query cooldown (24h) and per-server daily budget (3), and two taken-back forces cool a query down for a week through a WINDOWED history read - eligibility returns by the window sliding, never by clearing a flag (the #2677 lesson). The self-review verdict table ships complete and specced too (re-judge a force at +1h/+24h against the cpu/exec baseline it was sold on; take it back when it is not at least 25% better, because a force that buys nothing still pins a plan against future data change and has to pay rent) - settling the rules a live force will be judged by before anything can place one. @@ -3154,3 +3155,5 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#2823]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2823 [#2826]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2826 [#2827]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2827 +[#2840]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2840 +[#2841]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2841 diff --git a/Darling/Darling.Tests/SweepBodyDetachPolicyTests.cs b/Darling/Darling.Tests/SweepBodyDetachPolicyTests.cs new file mode 100644 index 0000000000..37c25297d8 --- /dev/null +++ b/Darling/Darling.Tests/SweepBodyDetachPolicyTests.cs @@ -0,0 +1,121 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System.Collections.Generic; +using System.Linq; +using PerformanceMonitor.Collectors; +using PerformanceMonitor.Darling.Service; +using Xunit; + +namespace Darling.Tests; + +/// +/// Pins the policy behind #2700/#2717 — WHICH collectors are detached from the sequential per-server +/// collection body, and the invariant that makes detaching safe. Neither had any test before #2840. +/// +/// Why the set is what it is, measured. The body runs every due collector for a server +/// together, and the outer launch loop will not relaunch it while it runs (INV-2, one body per server), +/// so one slow collector delays every other collector for that server. From +/// collect.collection_log on prod-pos-use1-monitor-01 over 24h (2026-09-03): +/// +/// +/// query_store — p50 3,350ms, p90 65,053ms, max 364,202ms +/// index_object_stats — p90 15,640ms, but 1440-minute cadence (42 runs/day +/// fleet-wide, one per server per day), so its body impact is amortised to nil +/// procedure_stats — p50 5,982ms, p90 11,964ms, but 1-minute tier +/// query_stats — p50 2,070ms, p90 5,062ms, 1-minute tier +/// plan_correction — p50 2,681ms, max 133,934ms — the same bimodal +/// shape as query_store with a smaller worst case, which is why #2717 followed #2700 +/// +/// +/// The criterion is measured p90 against the fast tier's cadence, NOT collector shape. +/// Enumerated-vs-scalar fanout was the intuitive rule and the data disproves it in both directions: +/// database_scoped_config fans out to 10 databases at p90 127ms, while procedure_stats +/// has zero fanout at p90 11,964ms. A fanout-derived split would detach the cheap collector and leave +/// the expensive one starving the tier. See #2840. +/// +/// The 4.5x evidence. use2 runs the same Balanced preset with Query Store dead since +/// 2026-08-17 17:36. Its query_stats delivered cadence stepped from 4.69-9.02 min (Query Store +/// live) to 1.44-1.57 min (dead) the following day, and held there for two weeks. +/// +public sealed class SweepBodyDetachPolicyTests +{ + /// The collectors #2700/#2717 detach, by the criterion documented on this class. + private static readonly string[] ExpectedDetached = { "query_store", "plan_correction" }; + + private static bool IsDetached(string name) => + DarlingWorker.IsQueryStoreCollector(name) || DarlingWorker.IsPlanCorrectionCollector(name); + + /// + /// The invariant that makes detaching safe, and the one that GENERALISES: a detached collector runs + /// fire-and-forget behind its own per-(server, collector) gate, so a still-in-flight run SKIPS rather + /// than overlapping. Skipping a 5-minute collector defers a tick; skipping a 1-minute collector is the + /// starvation this policy exists to prevent. So detaching anything on the 1-minute tier trades one + /// failure mode for another — it must not happen silently. + /// + /// This is what would fire if someone "fixed" the residual ~1.5-minute floor (#2841) by + /// detaching procedure_stats (p90 11,964ms, 1-minute tier) instead of addressing the body's + /// sequential execution. + /// + /// Scoped to FrequencyMinutes is > 0 and < 2: cadence 0 is the on-load-only tier, + /// which runs once per connect and so has no relaunch to skip — detaching one is harmless and this + /// invariant would misdiagnose it as a 1-minute-tier hazard. + /// + [Fact] + public void NoDetachedCollectorSitsOnTheOneMinuteTier() + { + /* > 0 deliberately: FrequencyMinutes 0 is the on-load-only tier (config snapshots), which runs + once per connect and has no relaunch cadence to skip against — so detaching one cannot cause + the starvation this guards, and flagging it here would report the wrong diagnosis. */ + var offenders = CollectorScheduleDefaults.All + .Where(kv => IsDetached(kv.Key) && kv.Value.FrequencyMinutes is > 0 and < 2) + .Select(kv => $"{kv.Key} (every {kv.Value.FrequencyMinutes}min)") + .OrderBy(s => s) + .ToList(); + + Assert.True( + offenders.Count == 0, + "A detached collector skips rather than queues when its previous run is still in flight, so " + + "detaching a 1-minute-tier collector converts starvation into guaranteed misses. Offending: " + + string.Join(", ", offenders)); + } + + /// + /// The detach set is exactly the measured cost outliers — asserted over the WHOLE catalog rather than + /// by naming the two, so a third collector added to the predicate fails here and sends its author to + /// the criterion on this class instead of to a name list. + /// + [Fact] + public void TheDetachSetIsExactlyTheMeasuredCostOutliers() + { + var actual = CollectorScheduleDefaults.All.Keys + .Where(IsDetached) + .OrderBy(n => n, System.StringComparer.OrdinalIgnoreCase) + .ToList(); + + Assert.Equal(ExpectedDetached.OrderBy(n => n, System.StringComparer.OrdinalIgnoreCase), actual); + } + + /// + /// Every name the detach predicate answers for must exist in the catalog. A predicate matching a name + /// no collector declares is dead code that reads as active policy — and both predicates compare against + /// the collector's OWN declared Name rather than a literal precisely so a rename cannot silently + /// unhook the detach, which this asserts is still true. + /// + [Fact] + public void EveryDetachedNameIsARealCatalogCollector() + { + foreach (var name in ExpectedDetached) + { + Assert.True( + CollectorScheduleDefaults.All.ContainsKey(name), + $"'{name}' is detached from the sweep body but is not in CollectorScheduleDefaults."); + Assert.True(IsDetached(name), $"'{name}' is expected to be detached but the predicate says no."); + } + } +}