Repository navigation
MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) - #5704
Merged
Merged
Conversation
…temp-table exclusion in get_table_index_sizes, Table not read class in get_index_usage (#5633 items 6, 15, 16)
…entory, Parallelism wait category (#5633 items 11, 20, 22)
…ead_latency percentiles above max (#5633 items 10, 12, 23)
…erver memory 0 vs 16 is a match (#5633 items 3, 19, 9)
…e dead CPU P95 and span reads, pin updates, CA1068
… as_of and keeps newer rows; Lite recommendations skip dropped databases (#5633)
…g thread; blocking_session_ids (#5633 L-a, L-c)
…icy job (#5633 item 14) A migrated store carries 142 policy jobs, all overdue at any restart, against the old hypertables + 2 (74) slots: 61 launch failures logged on one store. Workers are now 2 x hypertables + 16 (160 / 171 at 72 hypertables); compose, README and the runbook follow, and the conf migration still recognises the legacy blocks as ours.
…s writes collect.analysis_collection_caveats (#5633 item 13)
…old floor and the policy-job ceiling (#5633 item 14)
…tCollectionCaveats parameter (#5633 item 13)
…it_config latest lookup, clutter fleet anchor, database size column order, shared CPU stand-down predicate, trace flag empty path (#5633)
…s no longer read as bare database_states; trace-flag window test seeds runs that stored the flag (#5633)
…pty-trace-flag-runs SQL, p95Cpu24h rename
…ped databases they left out; the current-database lookup logs its fail-open (#5633)
… current-database lookup (#5633)
This was referenced Oct 13, 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.
Claude posting for Erik Darling.
Closes #5633.
This PR fixes the defects from the MCP tool audit in #5633. Several lanes build it on one branch. Each lane writes only its own section below, between its markers.
Items 13 and 14 changed files that PRs #5700 and #5697 also changed. Both of those have merged, so items 13 and 14 are in the second wave (W2A, W2B).
L1: advice text for spills, cross joins and anomaly sigma (items 2, 5, 8)
Why
QUERY_SPILLSisSUM(delta_spills)overv_query_stats(DarlingPgFactCollector.QueryPerf.csand LiteDuckDbFactCollector.QueryPerf.cs, same SQL). That column issys.dm_exec_query_stats.total_spills, which counts 8 KB pages, not spill events. The headline said "N query spills". The fact already carriesspilling_query_count(rows withdelta_spills > 0), so no collector or store change was needed.PLAN_WARNINGinvestigation.grep -rI joopsover the repo now finds nothing.ComposeAnomalyprinteddeviation_sigmabesidebaseline_mean. WhenAnomalyGate.Decideruns in the robust frame (bucket has a median or MAD), both sigmas are measured frombaseline.Medianin MAD-derived sigmas, and both are clamped atAnomalyThresholds.SigmaDisplayCap(25).What changes (all in
PerformanceMonitor.Analysis/FactAdvice.csunless noted)ComposeQuerySpills: headline and investigation now read "12,800 pages (100 MB) spilled to tempdb" (pages x 8 / 1024), via newSpilledPagesText. The static QUERY_SPILLS block said drill-down is "ranked by spill count"; it now says "ranked by pages spilled (8 KB each)".FactScorer.cs: comment only (the 100/1000 thresholds are pages). No threshold change.server-tabs.jscolumn labels "Total/Min/Max/Avg Spills" and the trend "Spills" are now "... Spilled Pages". JSON keys unchanged.ComposeAnomaly: newAnomalyUsedRobustFrame(median > 0 or MAD > 0, which is exactly whenEffectiveRobustSigmais positive; the detectors stamp both as zeros for classical-frame metrics). In the robust frame the sentence names "baseline median" and the median value and says "robust σ"; the classical frame keeps the old wording and the mean. NewSigmaText: a sigma at the cap reads "at least 25 robust σ" (constant, not a literal). Applied to peak sigma, window-mean sigma and the headline.ComposeAnomalyWaitProfile: the mean's "robust σ" clause goes throughSigmaText(cap handled).ANOMALY_MEMORY_PRESSUREgoes throughComposeAnomaly. Ratio, object-growth and object-contention composers print no sigma.delta_sigma, clamped byComparisonBanding): at the cap it now says "at least +25σ" / "at least −25σ".Darling/Darling.Tests/McpToolAuditFactAdviceTextTests.cs(8 tests).Test plan
Darling.Tests: 0 Warning(s), 0 Error(s).FactAdvice.csto the old text (plus a stub for the new helper): 7 of 8 new tests fail; the classical-frame control passes. Restored.-classMcpToolAuditFactAdviceTextTests, FactAdvice, Anomaly, Census, Ratchet, DocCommentHygiene, McpToolsListBudget: Total 731, Failed 1, Skipped 13. The one failure,SlowClassCensusTests.APullRequestIntoMain_SkipsNothing_AndOneIntoDevSkipsTheSameAsBefore, passes alone (43 s); it timed out under the load of the 283 s run.Lite parity:
FactAdviceandFactScorerare shared by Lite and Darling, so the composer fixes apply to both. Checked Lite-only text:Lite/Mcp/McpAnalysisTools.csQUERY_SPILLS recommendations say "Find queries with spills" (a query that spills, correct), LiteDuckDbFactCollector.QueryPerf.csbuilds the same page sum. Lite WPF grid headers ("Total Spills", "Min/Max Spills", "Spills Delta" inLite/Controls/ServerTab.xaml,Lite/Windows/*HistoryWindow.xaml, and the Darling Viewer twins) still carry the old noun; not changed here because they are many hand-written XAML headers with filter-button tags and sort keys.What the seat should double-check
total_spills/avg_spills/min_spills(contract change, deliberately untouched). Tool descriptions that mention "spills" name the DMV column, left as is.spilling_query_countcountsv_query_statsrows with spills, which the headline still calls "distinct queries"; not touched.baseline_median/baseline_mad; a fact from before Darling: anomaly-engine statistical depth (MAD/EWMA/changepoint; real confidence) #1743 (no such keys) renders the classical wording, same as before.Changelog:
SECTION: Fixed
ENTRY:
REF:
[MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704]: MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704
L2: truthful empty reasons for top procedures and trace flags, min server memory 0 vs 16 (items 3, 19, 9)
Why. Three MCP reads told the wrong story.
get_top_procedures_by_cpugave the "two collection cycles" reason for every empty page.get_trace_flag_changessaid "fewer than two config snapshots" withsnapshot_count0 after thetrace_flagscollector had run clean and stored nothing (no flag is on).min server memory (MB)configured 0 beside in-use 16 showedvalues_matchfalse, though SQL Server always runs 0 as 16.What changes.
PerformanceMonitor.Common/CollectorRunFacts.cs: the shared "collector ran and found nothing" evidence (successful runs capped at 2, runs inside the window, newest success time androws_collected) and the shared sentences. Darling reads it fromv_collection_log(DarlingCollectorRunFactsReader), Lite from DuckDB (LocalDataService.GetCollectorRunFactsAsync). Only SUCCESS rows count. The cap is a bound parameter (the terminal-LIMIT census forbids a literal).procedure_statsruns givesunavailablewith the two-cycles text (now also saying the collector has not completed two runs). Two or more runs with at least one inside the window givesempty: "No procedure executed in the last Nh that matches this request ...". Two or more runs but none in the window givesunavailable("no successful run inside the last Nh", a collection gap, not an all-clear). I added that third branch so "nothing ran" is never claimed for a window nobody measured.get_trace_flag_changesandget_trace_flags(both products): when the newest successfultrace_flagsrun stored zero rows, the answer is "No trace flags are enabled as of the trace_flags collector's latest successful run (T UTC)." The "fewer than two snapshots" text stays only when there is no such run (or the run stored rows).snapshot_countis unchanged (captures that listed a flag).PerformanceMonitor.Common/ConfigValuesMatch.cs;(0, 16)formin server memory (MB)only counts as a match. Used by Darling'sDarlingCurrentConfigReader.ServerConfigReadRow, the Darling viewer'sServerConfigRow, and Lite'sServerConfigRow. The web grid readsvalues_matchfrom the MCP payload, so it follows.ConfigChangeDiffdoes not compute this flag (it diffs changes), so I left it.McpToolsListBudgetTestspins did not move. get_collection_health note text and the expected-zero list were not touched.Test plan.
Darling.Tests.CollectorRunEmptyReasonTests: 16 passed.Lite.TestsCollectorRunEmptyReasonTests: 11 passed (DuckDB-backed: seedscollection_logand calls the real tools).HasDeltaBaseline/LastRunFoundNothingforced false and the match rule reduced to equality, 4 of 11 failed (after-run procedures, window gap, trace flags after an empty run, and the (0,16) case). Restored afterwards.McpToolsListBudgetand my class, before the last merge: 536 run, 1 failure (SlowClassCensusTests.APullRequestIntoMain_SkipsNothing..., which passed alone, 35 s; load-sensitive, unrelated). It had also caught two real problems in my first draft, both fixed: a literalLIMIT 2(census wants a bound cap) and my helper placed between a tool and its body (the bounded-parameter census lost the tool).origin/fix/5633-mcp-tool-audit(other lanes' code came in). Please rebuild both test projects; I expect no conflict, but it is unverified.DarlingCollectorRunFactsReader(SQL shape is pinned; both statements are plain reads ofv_collection_log). No rig was started.Lite parity. Item 3:
McpQueryTools.GetTopProceduresByCpu. Item 19:McpConfigHistoryTools.GetTraceFlagChangesandMcpConfigTools.GetTraceFlags. Item 9:LocalDataService.Config.csServerConfigRow. Lite's old trace-flag-changes text never said "fewer than two ... captured" with a count; it said "at least two snapshots are needed", which was also wrong after an empty run, so it uses the same shared sentence.What the seat should double-check.
procedure_statsoutcome (unavailable, runs exist but none in the window) is a behavior addition beyond the ruling.collection_logread on these empty paths now propagates to the tool'serrorenvelope rather than falling back to the old (false) text.Changelog:
SECTION: Fixed
ENTRY:
get_top_procedures_by_cpugave the "two collection cycles" reason for every empty page; it now says that only while the procedure_stats collector has fewer than two successful runs, and otherwise says no procedure executed in the window (or that the window was not measured).get_trace_flagsandget_trace_flag_changes(when the window holds no change) now say no trace flags are enabled when the trace_flags collector ran and stored nothing, instead of "fewer than two snapshots".min server memory (MB)configured 0 beside in-use 16 now shows as a match in get_server_config, the Darling viewer and Lite.REF:
[MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704]: MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704
L3: audit_config edition, compare_analysis overlapping windows, get_read_latency percentiles above max (items 10, 12, 23)
Why
audit_configprinted"edition": "Enterprise"for a Developer Edition server, because it named engine edition 3 (Enterprise, Developer and Evaluation all report 3).compare_analysisonly checkedbaseline_hours_back > hours_back, so the baseline window (same length as the comparison) could overlap it. The default stayed 28 whateverhours_backwas.get_read_latencyreturned p50/p95/p99 as histogram bucket upper bounds, so they could sit abovemax_ms(max 3,568 with every percentile 5,000).What changes
DarlingMcpTools.AuditConfig,McpAnalysisTools.AuditConfigin Lite):editionis now the storedEditionstring from the latest server-properties snapshot (the valueget_server_propertiesreturns). NewAuditEditionLabel(storedEdition, engineEdition)falls back to the engine-edition label only when the store has no Edition string, and then a newedition_notefield says it is the engine edition, not the SKU.edition_noteis null otherwise. No audit rule keys on the label (checked:editionNameis only echoed in the payload, and the pins forbid branching on edition), so Developer and Evaluation advice is unaffected. The tool description is unchanged.McpHelpers.ValidateBaselineWindow(PerformanceMonitor.Common), called by both products.baseline_hours_backis nowint?. Omitted:hours_back + 24(4 still gives 28), raised to2 x hours_backwhen that is larger (hours_back above 24), and refused with a "no room" message pasthours_back84 because the 168-hour ceiling cannot hold a disjoint baseline. Explicit and below2 x hours_back: refused as overlapping, naming both windows ("the last 4 hours" and "hours 7 to 3 back", baseline_hours_back 7 is below 2 x hours_back (8)). Darling web: thecompare_analysisroute now passesnullwhen the query string omitsbaseline_hours_back(newQueryNullableInt), and the catalog entry no longer advertises a default of 28. Tool head now reads "(>= 2 x hours_back, else refused)" and the parameter text "Baseline start, hours back from the comparison's end. Default hours_back + 24; at least 2 x hours_back." (shorter than before, so the budget pins went DOWN: tool 611 to 606, parameter 139 to 103, in both products). The guide tail states no default, so it is unchanged.ReadLatencyPercentiles.FromBucketstakes an optionalmaxMs; a bucket bound above it is capped to it. This is the only caller path (MCP tool; the web route calls the tool), and Lite has noget_read_latency. A capped value is no longer a bucket bound but is still an upper-bound estimate (the true percentile is at or below max), so*_is_at_leaststays false and the overflow-bucket "at least" floor is untouched.BucketEstimateNoteand the tool guide now say a bound abovemax_msis capped tomax_ms. No new response keys.AuditConfigEditionNameTestssource pin,McpToolGuideHeads.SqlCore.cs(Darling and Lite) guardrail fact,McpPayloadContractCensusTestsshared-producer regex (addsValidateBaselineWindow),GetReadLatencyLiveTests(p99 90,000 becomes 61,000 for max 61,000; p95/p99 100 becomes 90 for max 90), LiteAuditConfigEditionTests(was asserting the engine name over a seeded Edition of "Test Edition"; now asserts the stored string and a null note).Test plan
McpToolAudit5633L3Tests(Darling and Lite): stored edition reported, fallback label plus note, source pins on the tool bodies,ValidateBaselineWindowaccept/refuse/default theories, percentile cap (3,568 case: old shape gives the bucket bound above max, new gives 3,568), bound at or below max untouched, overflow floor kept.*Census*,*Ratchet*, DarlingWebEndpointsTests, McpToolGuide*, DarlingCustomViewsTests, AnalysisAsOfAnchorTests).*AuditConfig*plus the new class, 13 total, 0 failed. Lite budget, guide, Census, Ratchet and truth-pin classes ran once earlier with only theAuditConfigEditionTestsfailures that I then fixed.AuditConfigEditionTestsrun failed with "Test Edition" vs "Enterprise" on the old expectation, showing the tool now reports the stored string. I did not revert the production code to prove the other pins.origin/dev(code from other lanes only), I rebuilt Darling.Tests and Lite.Tests (0 warnings) but hit the 60-minute wall before re-running the test classes. Please re-run the budget, guide-head, census and new classes.Lite parity:
audit_config(same edition fix andedition_note),compare_analysis(shared validator, same description and param text, same pins) done. Lite has noget_read_latency, so item 23 is Darling only. Checked by grep:ReadLatencyPercentilesandBucketUpperBoundsMshave no other caller in Lite, the viewer, or the web.What the seat should double-check
max(hours_back + 24, 2 x hours_back)for the omitted default, not barehours_back + 24, because past 24 hours the bare default would refuse the caller's own omission. Identical to the ruling forhours_backup to 24.audit_configadds one store read (latest server properties) and one JSON key,edition_note; the PostgreSQL branch is untouched.max_ms >= 0; a stored max of 0 would cap percentiles to 0 (true: nothing took longer than 0 ms).DarlingWebEndpoints.cs(route parse and catalog default only) andMcpPayloadContractCensusTests.cs(one regex word). Not in the PR Role provisioning stops before it sets a password when it cannot change the roles; store connection strings are reported without their values #5697-MCP and web host defaults: form sign-in, token length floor, sign-in wait, write-tool token gate #5702 key list.Changelog:
SECTION: Fixed
ENTRY:
REF:
[MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704]: MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704
L4: percent growth, NULL-last table sizes, heap and clustered usage class (#5633 items 7, 6, 15, 16)
Why. Item 6: get_database_sizes published only auto_growth_mb, so a percent-growth file read as null. Item 15: get_table_index_sizes sorted
current_reserved_mb DESCwithout NULLS LAST (PostgreSQL puts NULLs first), showed an unsized table as 0, and listed the collector's own tempdb#tables. Item 16: get_index_usage labelled heaps and clustered indexes "Unused", a drop-candidate label for the table itself. Item 7 was verify-only.What changes.
Lite.Tests/CollectorSelfQueryMarkerCensusTestsbuilds every SQL Server collector query over six target shapes and also checks each innersp_executesqlbatch on its own; it passes (ran in the Lite census set).DarlingSelfQueryMarkerTestscovers Darling's by-id Query Store fetches, detection query and job reads. No statement was missing the marker; no code change.DarlingObjectStatsReader.DatabaseSizeLatestSqland theDatabaseSizeRowrecord gainis_percent_growthandgrowth_pct;DarlingMcpObjectStatsTools.FilePayloadpublishes both. Lite:LocalDataService.GetLatestDatabaseSizeStatsAsync,DatabaseSizeStatsRowandMcpServerInfoTools.FilePayloadthe same.ObjectSizeGrowthSql(Darling) and the DuckDB twin ordercurrent_reserved_mb DESC NULLS LASTplus database, schema and table names as tie-break, and drop#-prefixed tables from the latest-snapshot CTE.CurrentReservedMbandCurrentUsedMbare now nullable on both products' rows, so the payload says null, not 0.IndexObjectStatsCollectoraddsAND o.name NOT LIKE N'#%'(shared by Lite and Darling); stored rows stay.CLUSTERED%index (includes clustered columnstore) with no reads isTable not read; with reads it falls to Active. Nonclustered classes unchanged. Applied in Darling'sIndexUsageSql, Darling's FinOps storage-growth reader, and both Lite CASEs (usage and per-table detail). The unused-first sort key now ranks only nonclustered zero-read indexes first, so table storage cannot crowd drop candidates out of the cap.IndexUsageTruncationTestspins that key, updated deliberately for this. Tool text (tail only, head untouched so the 493-char cap andMcpToolsListBudgetTestspins did not move): the classification rule names Table not read; the reset caveat now reads "clears on an instance restart, a database restore, a detach and attach, the database going offline, and (for that index) a rebuild"; the "cumulative since the last instance restart" sentence is softened. Per-database counter age is NOT reported: the store holds only the instance start time (sqlserver_start_time), no per-database one.Test plan.
Darling.Tests/McpToolAudit5633Tests.cs(4 pins plus 3 live tests on a UTC rig, port 5684) andLite.Tests/McpToolAudit5633Tests.cs(5 tests on DuckDB). Reverted the NULLS LAST,#filter and Table not read pieces once: 3 Darling tests failed (SQL pin, table sizes live, usage live); restored, all pass.*ObjectStats*,*Census*,*Ratchet*, DocCommentHygiene: 656 passed, 0 failed. Earlier broader set (McpToolGuide, IndexUsage, StorageGrowth, DatabaseSize, ZeroIsAMeasurement): 921 total, the 2 failures (truncation pin, guard-stage trait on my new class) fixed.*McpToolAudit5633*in both.Lite parity. Items 6, 15, 16 each done in Lite (reads, payload, tool text); item 15's collector filter is shared code. DuckDB already sorts NULLs last by default; NULLS LAST made explicit.
Double-check.
#exclusion also applies to any database, not only tempdb (a user table whose name starts with#would vanish from the read).Table not readis a new classification value; any client matching only Unused/Write-only/Active sees it for heaps and clustered indexes. The Viewer grid shows the class text as is.C:/Users/edarl/AppData/Local/Temp/pmrig-5684is still running (lane hit the wall-clock tool lock before it could stop it). Stop it withpg_ctl stop -D .../pmrig-5684/data.Changelog:
SECTION: Fixed
ENTRY:
REF:
[MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704]: MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704
L5: one CPU right-sizing verdict, null inventory cost, Parallelism waits (#5633 items 11, 20, 22; fixes #5644 W18)
Status: Darling and Lite are done and green. This lane hit its 60-minute wall before the Lite tests; F1 updated them (see F1).
Why
What changes
FinOpsRecommendationFigures.CpuVerdictAllowsDownsizing(util)is true only when the Utilization read has a CPU sample and its own verdict is OVER_PROVISIONED.CpuRightSizingTargetCores(the compute row) requires it, and the reader's VM row requires it too. The VM CPU row now reads the Utilization view's 24-hour window and P95 (the samevmUtilread), not a separate 7-day P95; its text says "the last 24 hours". Both rows wait forCpu24HourWindowWatched. The existing "the row that keeps more cores wins" rule is unchanged, so the list still holds one CPU row. The memory row keeps its 7-day window.ServerHardwareScope.CpuCoreNounnow returns "logical CPUs" off Azure SQL Database (vCores stays on it); the VM row text says "logical CPUs" too.monthly_cost_usdandannual_cost_usdnull (left out of the row) andcost_reason"monthly cost not set" when the cost is 0. Guide text updated.FleetHealthScoreForand the Utilization tool use the same inputs and window (24-hour P95 CPU, the newest memory row's buffer pool over physical memory, the newest database-size snapshot's free share) and the sameFinOpsHealthCalculator. The code already agreed (fixed earlier in b5bc69a, before the audit's base), so I could not reproduce 92 versus 98 without the audit's store. The guide now says the score is from the same inputs and that two calls made across a collection can differ by what that collection changed. If the seat sees a gap on a quiet store, that is a real bug I did not find.DarlingFinOpsOptimizationReader.WaitCategorySummarySqlnow maps SOS_SCHEDULER_YIELD to CPU and CXPACKET, CXCONSUMER, CXSYNC_PORT, CXSYNC_CONSUMER, HTDELETE, HTBUILD, HTREPARTITION to "Parallelism". Theget_finops_optimizationguide lists the categories.Test plan
CpuRightSizing_FollowsTheUtilizationVerdict_SoItCannotDisagreeWithIt(RIGHT_SIZED at P95 24% gives no row),Row_WithNoCostSet_IsNullWithTheSameReasonTheOtherFinOpsToolsGive, liveCpuRightSizing_FollowsTheUtilizationVerdict_OneRowOneWindowOneP95, and the seeded wait test now has HTBUILD/HTREPARTITION/HTDELETE and expects Parallelism first (CXPACKET is no longer CPU). GoldensFinOpsOptimization/golden.jsonandFinOpsRecommendations/golden.jsonregenerated (the recommendations golden loses the two CPU rows, because the seed's 22% CPU is RIGHT_SIZED).-class "*Census*"and-class "*Ratchet*"in both projects; the whole Lite.Tests project (Lite production code was edited but not built or run).McpToolsListBudgetTestswas in the run list for Darling and passed, but the guide text grew by about 600 characters; confirm the total ceiling on CI.Lite parity
Lite/Services/LocalDataService.FinOps.Recommendations.cs: rule 2 requires OVER_PROVISIONED; rule 12 CPU row usesvmUtil.P95CpuPct, requires OVER_PROVISIONED and the 24-hour watch, 7-day CPU query andcpuSpanEnoughremoved; "logical CPUs" text.Lite/Services/LocalDataService.FinOps.Workload.cs: both wait-category CASE copies now have Parallelism.CpuCoreNounis shared (Common).CpuSamplesSpanEnough, DarlingGetCpuP95Async/GetCpuSampleSpanAsync/CpuSamplesSpanEnough). Lite'sAzureSqlDatabaseOwnFiguresTestsnoun pin was already updated; the Azure vCores tests were unaffected.What the seat should double-check
Changelog:
SECTION: Fixed
ENTRY:
REF:
[MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704]: MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704
L6: dropped databases are no longer reported as current (item 4)
Why. The Query Store clutter tool listed 13 databases, several dropped, one with a Critical verdict. FinOps recommendations and index analysis listed dropped databases, "compress
dbo.r" named no database, andget_top_procedures_by_cpuover 168 hours returned only a dropped database's rows. The backfill retry part was already fixed by #5486.What changes.
CurrentDatabaseScope(Darling.Storage): the current-database set is the newestdatabase_statessnapshot per server, read withQueryStoreBackfillState.NewestSnapshotSql, the same SQL text The Query Store backfill stops retrying databases that were dropped from the server (#5483) #5486'sDropGoneDatabasesAsyncuses (no second source). It returns null (filter nothing) with no snapshot (older store, Azure SQL Database), a snapshot older than 6 hours, a snapshot with no names, or a failed read.GetReadCostAsync,GetPlanChurnAsync,GetConfigAsync): rows of a dropped database are OMITTED when no database is chosen, so a dropped database cannot carry a verdict, a count or a recommendation. Omitted rather than markeddatabase_dropped, because the verdict counts and fleet rollups are built from these rows and a mark would need every consumer to learn it.ReadIndexObjectStatsAsync, the one read the analysis, the desktop tab and the compression rule share): rows of a dropped database are omitted (same reason).SalesA.dbo.T1 (2.0GB), falls back toschema.tablewhen the input has no database). Golden fixture updated for it.get_top_procedures_by_cpu(Darling): with no database named, the filter is narrowed to the current set, so the SQL is unchanged and the rank's LIMIT no longer fills with a dropped database. A database the caller NAMES is read as asked (a question about its history).Plan cost. The lookup is one indexed read per call (
MAX(collection_time)onidx_database_states_time (server_id, collection_time)with a 6-hour lower bound, then one row per database). It never touches a fact table. I did not run EXPLAIN on a large store; no fact-table SQL was changed.Lite parity. Lite's
get_top_procedures_by_cpugets the same narrowing (LocalDataService.GetCurrentDatabasesAsync,v_database_states, same SQL text and horizon). NOT done (parked, out of time): Lite FinOps has no equivalent of Darling's index analysis read; its compression rule is a livesys.partitionsscan of the monitored server, which cannot return a dropped database, and its recommendation text names no database there either (same text "schema.table"; left as is). Lite has no clutter tool I checked.Test plan.
DroppedDatabaseScopeLiveTests(4 tests, rig port 5686): FinOps (no snapshot, stale snapshot, newest snapshot dropping two databases; recommendations, index inputs, index analysis rollups), top procedures (top 1 page, named dropped database, no snapshot), clutter config and plan churn, rule unit tests, compression text. Total 4, 0 failed. Proven to fail on the old shape: withReadAsyncforced to return null, 2 of 4 fail.CurrentDatabasesTests1 pass; Lite*Census*67 pass;DatabaseStateNoVerdict10 pass.*QueryStoreClutter*67 pass,*TopProcedures*16 pass.*FinOpsRecommendations*: 87 of 89 passed; the 2 golden tests failed only on the newSalesA.prefix; fixture updated in the second commit and the golden class re-run: 2 of 2 pass.*FinOpsIndexAnalysis*50 of 50 pass.*Census*,*Ratchet*,*StorageCommandTimeout*,*DocCommentHygiene*,*LivePostgresCollectionHygiene*: started, results not read before the wall. CI decides.origin/fix/5633-mcp-tool-audit(no conflicts) but did not mergeorigin/devor rebuild after it.Seat should double-check. (1) The golden fixture edit (
golden.json, server a). (2) Any Viewer test asserting compressionDetailwith a non-empty database name. (3) A database created after the newest snapshot is hidden until the next snapshot (30 minutes by default); the backfill's "row since the snapshot" guard was not copied. (4) Lite index/FinOps parity above.Changelog:
SECTION: Fixed
ENTRY:
SalesA.dbo.T1).REF:
[MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704]: MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704
L7: panel server names, waiting-task rollup, plan DOP (items 17, 18, 21)
Why
run_custom_view_panelmatched the spec'sserveronly against the storedserver_name, so a display name returned 0 rows. Re-checked on current code: the unknown-server half is already answered by W13 (not_found, "No server named 'X' is registered", pinned inDarlingMcpCustomViewToolsLivePostgresTests), so only the display-name half was still broken.get_waiting_tasksreturned one row per waiting thread, so a parallel query looked like 30 duplicates, 29 "blocked by" their own session.QueryPlan/@DegreeOfParallelism; the parser cast the missing value to 0, so the three plan tools saiddop: 0for a parallel plan.What changes
ComposeServerScope.ResolveNames[Async]runs once per panel run (shared runner, so MCP and web alike) and places each scoped name throughDarlingServerResolver.ResolveOrError(exact, display name, partial, case-insensitive). An exact storage name is kept as written. A name matching several servers is refused with the resolver's own sentence. A name matching nothing is left as written, so a removed or disabled server's stored history (Custom Views and custom alert rules filter raw reads by server name, so every compressed chunk is read in full #5525) and W13's unknown-server answer both keep working. Registry read fault: fail open, like the unregistered-name lookup.WaitingTasksSql(Darling) and Lite'sGetWaitingTasksAsyncgroup by capture, session, wait type, database:wait_duration_msis the max,thread_countis new,blocking_session_idis only a blocker other than the session itself, and newintra_query_parallelismflags threads waiting on their own session. No new column, no migration. Tool description (both products) gets one GUIDE sentence.StatementInfo.HasDegreeOfParallelism(new); the formatter emitsdopas null when the attribute is absent and a newparallelflag from operators markedParallel="1". The intDegreeOfParallelismstays 0 when absent, so every analyzer/benefit/UI reader (> 1,> 0,== 2,stmt.DegreeOfParallelism > 0 ? ... : 1) behaves exactly as before. PlanAnalyzer.cs and PlanRegex.cs are untouched.Test plan
PlanDopParallel5633Tests(4),WaitingTasksRollup5633Tests(SQL pin + live),ComposeServerScopeResolveNamesTests(4), liveCustomViewTools_CrudAndRunRoundTripextended (display name and upper-case spelling return rows): 16/16 pass on a UTC rig (port 5687), after merging the shared branch.*Census*,*Ratchet*, McpToolGuide, McpPageContract, PlanSync*, McpPlanAnalysis*, BenefitScorer*, DarlingMcpPlanTools*: 1168 passed, 0 failed (before the final merge). The description change fit the existing budget pins; none changed.WaitingTasks_ParallelQueryThreads_...plus WaitingBlockedDatabaseFilterToolTests, McpToolsListBudget, Census, Ratchet, McpPageContract, McpToolGuide, McpWindowNoticeTool, DataStartBanner: 453 passed, 0 failed (before the final merge).origin/devmerge (shared branch merged twice, dev not merged for time), Lite rebuild after the final merge.Lite parity
v_waiting_tasks, same grouping,McpWaitToolsoutput, twin test, same description sentence).Parked
validate_custom_viewflagging an unknown server: a view definition carries no server scope (the server is the run spec'sserveror the$servervariable at run time), and the tool is a pure, store-less validator. Flagging would need a store parameter on the tool, which touches the DI-seat census. Not done; the run itself already answers unknown names.not_found(only when nothing is stored under the name), not by an up-frontinvalidrefusal, to keep Custom Views and custom alert rules filter raw reads by server name, so every compressed chunk is read in full #5525's removed-server history readable. Say if the seat wants the harder refusal.EffectiveDOPstill casts a missing attribute to 0; nothing emits it (UI shows it only when > 0), so no payload change was needed.What the seat should double-check
get_waiting_tasksrow counts andlimitmeaning (the cap now counts rolled-up rows); the aggregate runs over the whole window before the cap.Changelog:
SECTION: Fixed
ENTRY:
REF:
[MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704]: MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704
F1: make the merged branch build and pass (integration lane)
Why. Seven wave-1 lanes merged onto one branch. Lane L5 changed the CPU right-sizing rules (Utilization verdict gate, one 24-hour window and P95, "logical CPUs" wording) and left Lite's tests, dead readers and a few pins behind; other lanes' new rows and new test class broke Darling tests and a census. Merging
origin/devwas a no-op (already up to date at head aa7415e).What changes (head 3a2729e, three commits, pushed to
fix/5633-mcp-tool-audit)CurrentDatabaseScope.ReadAsync(CancellationToken not last).nowUtcnow comes before the token; the two internal callers andDarlingFinOpsRecommendationsReaderpasscancellationToken:by name.FinOpsTests: "cores" became "logical CPUs" in the four CPU-row assertions, the Azure no-vCores check also rejects "32 logical CPUs", and the Azure vCores test also asserts the text does not say "logical CPUs". The two tests that asserted the removed 7-day detail are replaced:VmRightSizing_AWeekOfSamples_CpuDetailNamesTheUtilizationWindow_NotTheSevenDaysObserved: a week of sparse samples plus a covered day; the detail starts "From the last 24 hours, P95 CPU" and has no "samples over".VmRightSizing_BusyClusterFourDaysAgo_DoesNotMoveTheTwentyFourHourP95_SoItCannotDisagreeWithTheUtilizationView: 90% CPU four days ago, 8% in the last day; the row must quote P95 8.0%. A separate 7-day read would blend the busy cluster in.CpuSamplesSpanEnough_NeedsAFullDay(covered only the dead helper).GetCpuP95Async,GetCpuSampleSpanAsync,CpuP95Sql,ViewerDataService.RecommendationsCpuP95Sql,FinOpsRecommendationFigures.CpuSamplesSpanEnoughandCpuRightSizingMinSpan.CpuSamplesSpanEnough,CpuRightSizingMinSpan,GetCpuSampleSpanAsync.FinOpsRecommendationsReadsLiveTests,CpuRightSizing_NeedsAFullDayOfSamples, theCpuP95SqlInlineData rows and one delegate assertion.RightSizingWindowTests.ViewerRightSizing_...: the "no literal 'the last '" guard now allows exactly the one"the last 24 hours"window the CPU row names, and nothing else.RightSizingWindowTests.Readers_TakeTheWindowFromThoseColumns_...: renamed..._AndTheCpuRowNamesTheUtilizationWindow; it assertsvar cpuWindow = "the last 24 hours", no "recent samples", nocpuReader.FinOpsTabServerInventoryPageTests.EveryShownColumnKeyIsEmittedByTheInventoryRow: 27 to 28 emitted keys (item 20 addedcost_reason).FinOpsOptimizationViewLiveTests: wait category rows 5 to 6 and 9 to 10 (item 22 added Parallelism as its own category).WaitingTasksRollup5633Tests(live, shared store) was missing[Collection("live-postgres")];LivePostgresCollectionHygieneTests.EveryClassUsingTheSharedStore_IsSerializedOrDocumentsWhyNotfailed on it. Added.WaitCategorySummary_ParallelismWaits_AreTheirOwnCategory_NotCpuOrOther(Lite had the Parallelism SQL change from item 22 with no test).Revert proofs (item 4). Reverted the product change once, rebuilt, ran, restored (
git checkout --on those files, then rebuilt):!= NotApplicable:CpuRightSizing_FollowsTheUtilizationVerdict_SoItCannotDisagreeWithItand..._OneRowOneWindowOneP95(live) both FAIL.monthly_cost_usd = s.MonthlyCost:Row_WithNoCostSet_IsNullWithTheSameReasonTheOtherFinOpsToolsGiveFAILS.OneRowPerSection_ReadsFieldByField_AgainstLiteralValues,HoursBack_WidensWaitsAndQueries..., plus the count tests).Test plan
*McpTool*,*Hygiene*,*Timeout*: 2183 run, 1 failed (the collection attribute above), then fixed. Hygiene, DocComment, Pin, Meta, AlertReadFailureSurface, ViewerSchemaVersion, Inventory, Ratchet, WaitingTasksRollup5633 after the fix: 942 run, 0 failed, 2 skipped.FinOpsOptimizationViewLiveTests(three count assertions, plusReadRoute_ReturnsTheToolsBody).ReadRoute_ReturnsTheToolsBodypassed alone and in the later full run; it compares two reads with a 10 second tolerance oneffective_start, so I read it as load-sensitive on this laptop, not a code failure. Not seen failing a second time.Line endings (item 6). For every file the branch changes against
origin/dev(72 modified files), compared the CR/LF mix of the blob on the branch with the blob on dev: no file flipped. No added or modified blob is CRLF or mixed (git ls-files --eol), and no file has a whole-file diff.Lite parity. Dead-code removal in both products; Lite FinOps tests brought to the new rule; Parallelism wait category now pinned in Lite as well as Darling.
What the seat should double-check
CurrentDatabaseScope.ReadAsyncparameter order changed (only caller is the recommendations reader; passcancellationToken:by name).ViewerDataService.RecommendationsMemoryP95Sqland the memory reads alone (still used, 7-day by design).ReadRoute_ReturnsTheToolsBodyfailure: say if you want that tolerance widened; I did not touch it.Changelog:
(none: test, pin and dead-code changes only; no user-visible change beyond what the other lanes' entries already name)
F2: Compose server scope is exact-match only; the current-database set honours as_of and keeps newer rows; Lite recommendations skip dropped databases
Fixes review round 1 findings M1, M2 (plus the new-database leftover from L6), M3 and L-b, and verifies the item 17 ruling.
Why
get_top_procedures_by_cpuwith anas_ofdays back hid a database dropped since. Also a database created after the newest snapshot (up to 30 minutes old) was hidden by the clutter reads until the next snapshot.CollectorRunFactsused the host culture's time separator.What changes
ComposeServerScope.ResolveNames: a scoped name resolves only by an exact match (storedserver_name, then display name, case-insensitive). No exact match leaves the name as written (the Custom Views and custom alert rules filter raw reads by server name, so every compressed chunk is read in full #5525 history path). Several exact matches still get the resolver's refusal. Item 17 check: an unknown name goes through the shared run path (ComposeServerScope.FindUnregisteredAsyncthenDarlingWebEndpoints.UnknownServerMessage), which returnsnot_foundnaming the name; the web panel run and the MCPrun_custom_view_panelboth use that one runner, so both give the message (testUnknownName_IsReportedNotFound_NamingIt).CurrentDatabaseScope: newReadSnapshotAsync(names plus the snapshot's collection time);ReadAsyncandNarrowAsynctakeasOfUtc. With an anchor the read uses the newQueryStoreBackfillState.NewestSnapshotAtOrBeforeSql(the same text pluscollection_time <= $3) with the 6-hour horizon measured from the anchor.DarlingMcpDataToolspasseswindowEndonly whenas_ofwas supplied.OmitDroppedAsynctakes an optionaltimeOf; a row stamped after the snapshot is kept (no extra query). Wired for the three clutter reads (last slowest / last collection / captured at) and the index-analysis read (collection time). Parameter order follows the branch's CA1068 rule (cancellation token last).GetCurrentDatabasesAsync(serverId, nowUtc, asOfUtc)uses the same at-or-before SQL againstv_database_states;McpQueryToolspasses the anchor whenas_ofis set.GetRecommendationsWithNoteAsyncreads the set once and drops dropped databases from the idle-database rule (6) and the low-IO-latency rule (13), unknown means no filter, name-only like Darling's reader.CollectorRunFacts: both stamps format withCultureInfo.InvariantCulture.Test plan
ComposeServerScope*,CollectorRunEmptyReasonTests,DroppedDatabaseScopeLiveTests(live, rig port 5682, UTC, not skipped),ComposeUnknownServerMessageTests, plus every*Census*,*Ratchet*,DocCommentHygiene,AlertReadFailureSurfaceTests,LivePostgresCollectionHygieneTests,StartupCommandTimeoutTests,StorageCommandTimeoutTests,*QueryStoreBackfill*,*FinOpsRecommendations*: 871 run, 1 failed. The failure wasLivePostgresCollectionHygieneTestsonWaitingTasksRollup5633Tests(another lane's class, missing its collection attribute); the branch has since taken a fix for it in a later merge, not re-run here.CurrentDatabasesTests, every*FinOps*,*Census*,*Ratchet*,McpToolsListBudget*,DocCommentHygiene: 254 run, 0 failed.--no-incremental, 0 Warning(s) (a first CA1068 warning onOmitDroppedAsyncwas fixed).Expected ["LiveDb","NewDb"], Actual ["LiveDb"]), Lite M3 (both new recommendation tests fail). The LiteAsOf_UsesTheSnapshotAtOrBeforeItand the liveReadAsync(asOfUtc)assertions use a new parameter, so on the old shape they fail to compile rather than at runtime.Lite parity
Checked each: M1 (compose is Darling only; Lite has no compose), M2 as_of (Lite
McpQueryToolsandGetCurrentDatabasesAsyncchanged), M3 (Lite is where the bug was; rules 1, 4, 5, 7 are live queries and already current, so only 6 and 13 needed it), L-b (shared Common file, both products). Item 3's keep-newer-rows is Darling only: Lite's set is applied as a name filter before the ranking read and as a name filter on the two recommendation rules, as Darling'sNarrowAsyncand recommendations reader do, and Darling's recommendations reader is also name-only, so Lite omits no rows by timestamp.What the seat should double-check
NarrowAsyncfilters by name before the read); only the omitted-row reads keep it. Stated in the class comment.NewestSnapshotAtOrBeforeSqlis a new const beside the shared one inPerformanceMonitor.Collectors/QueryStoreBackfillState.cs; the backfill itself still uses the original.Changelog:
SECTION: Fixed
ENTRY:
as_ofset, the "databases the server has now" list came from today's snapshot, so a database dropped after the window vanished from the ranking. It now uses the newest snapshot at or before theas_oftime, in Darling and Lite. In the Query Store, FinOps index and configuration reads, a database created since the last snapshot is no longer hidden until the next one.REF:
[MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704]: MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704
F3: waiting-task rollup from one thread, source-grep tests replaced, spilled-pages wording
Covers #5633 items 2, 12 and 18 (review round 1 lows L-a and L-c, and lane L1's item 2 leftovers).
Why
MAX(wait_duration_ms),MAX(resource_description)andMAX(blocking_session_id)independently, so a row could show one thread's wait beside another thread's resource and hide a second, lower-numbered blocker.DarlingMcpTools.cscontained certain strings, so a refactor that kept the text but broke the wiring would pass.sys.dm_exec_query_statsspill columns that count 8 KB pages; the MCP guide text did not say pages; andspilling_query_countwas read as "distinct queries" by the advice text but was a row count.What changes
get_waiting_tasks, both products.wait_duration_ms,resource_descriptionandblocking_session_idnow come from the longest-waiting thread of the group (Darling: orderedarray_agg; Lite:arg_maxover a struct, so a NULL resource or blocker on the winning thread is not skipped). Ties break on the lower blocker, then the greater resource text, the same way in both products. If the longest thread waits on its own session (parallelism),blocking_session_idis the blocker of the longest-waiting thread that has another session as its blocker, so a real blocker is never dropped. Newblocking_session_ids(sorted distinct blockers other than the session itself), set only when there is more than one, otherwise null.thread_countandintra_query_parallelismare unchanged. Guide tail of the tool says so; the served head did not change.AuditConfig_ReadsTheStoredEditionPropertyandCompareAnalysis_NoLongerDefaultsBaselineHoursBackTo28are now live tests that callAuditConfigandCompareAnalysison seeded PostgreSQL data. The audit test storesDeveloper Edition (64-bit)with engine edition 3 and expects that string and noedition_note. The compare test expects the overlap and "no room" refusals, and withhours_back24 and no baseline expects the baseline to start 48 hours before the anchor (and not 28).ServerTab.xaml, Lite and Darling ViewerProcedureHistoryWindow.xamlandQueryStatsHistoryWindow.xaml,ViewerServerTab.xaml) now say "Total/Avg/Min/Max Spilled Pages", "Spilled Pages" and "Spilled Pages Delta", matchingserver-tabs.js. Only header text and, for the longer labels, column widths changed; every binding path, sort tag and filter tag is as before.get_top_queries_by_cpuguide tail (both products) saystotal_spillscounts 8 KB pages written to tempdb, not spill events (Darling'sdetail='full'sentence too). The JSON keys are unchanged.spilling_query_count: it wasCOUNT(CASE WHEN delta_spills > 0 ...)overv_query_stats, one row per query per snapshot, so one query spilling in thirty snapshots counted thirty times and the investigation text said "across 30 distinct queries". I chose to count distinct queries rather than reword the text, because the text and the fact name already say distinct queries and the drill-down groups by (database, query_hash): it is nowCOUNT(DISTINCT database_name || '/' || query_hash)over rows with a spill, in both collectors. A row with a NULL hash counts once per database.Test plan
WaitingTasksRollup5633Tests,McpToolAudit5633L3Tests,QuerySpillsDistinctQueryCountLiveTests,McpToolsListBudget*, every*Census*and*Ratchet*class,McpToolAuditFactAdviceTextTests,ViewerHistoryWindowTests: 572 run, 0 failed, 0 skipped (live rig on port 5683, UTC, both preload libraries).WaitingBlockedDatabaseFilterToolTests,QuerySpillsDistinctQueryCountTests,McpToolsListBudget*, every*Census*and*Ratchet*class: 90 run, 0 failed. Earlier, with*FactCollector*and*QueryHighDop*added: 169 run, 0 failed.MAX(resource_description)back in the Darling SQL,DisagreeingThreads_...and the SQL-text test fail; withAuditEditionLabel(null, edition)inAuditConfig,AuditConfig_EchoesTheStoredEditionString_...fails; with the old row count, both spill-count tests fail with 4 against the expected 2.Lite parity
LocalDataService.WaitingTasks.cs(DuckDBarg_max) andWaitingTaskRow.BlockingSessionIds,McpWaitTools.GetWaitingTaskspayload and guide; testWaitingTasks_DisagreeingThreads_...inWaitingBlockedDatabaseFilterToolTests.ServerTab.xamlplus its two history windows,McpQueryToolsguide,DuckDbFactCollector.QueryPerf.cs, testQuerySpillsDistinctQueryCountTests.What the seat should double-check
blocking_session_idsis always present in the payload and null unless there is more than one other blocker. If you would rather omit the key when null, say so.blocking_session_idandresource_descriptioncan then come from different threads (the resource stays the longest thread's). I judged showing a real blocker better than hiding it.server-tabs.jswas only read for its labels; not touched.Changelog:
SECTION: Fixed
ENTRY:
REF:
[MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704]: MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704
F4: turning off the last enabled trace flag now shows as a change (item 19)
Why.
ConfigChangeDiff.DiffTraceFlagChangesset-diffs consecutivetrace_flagscaptures, but the collector stores one row per enabled flag and nothing at all when none is enabled. A run that finds no flag therefore left no capture to compare, so turning off the LAST enabled flag (and turning on the first one after an empty run) never appeared as a change, in either product, in the tool, the UI or the analysis findings.What changes.
PerformanceMonitor.Common/ConfigChangeDiff.cs: new overloadDiffTraceFlagChanges(snapshots, emptyRunTimes, start, end). Each time inemptyRunTimesis an empty snapshot (a capture with no flags); a time that already holds flag rows is ignored. The old two-argument signature delegates with an empty list, so nothing else changes.collection_logfortrace_flagswithrows_collected = 0(a failed run, or a NULL count, is never an empty snapshot). The read is bounded like the change read: the runs inside the window plus the single newest run before the window (the baseline for a change on the window's first capture), never a scan of the log.get_trace_flag_changes:DarlingCollectorRunFactsReader.ReadEmptyTraceFlagRunsAsync.ViewerDataService.GetTraceFlagChangesAsync(EmptyTraceFlagRunsSql).DarlingAnalysisService.EmptyTraceFlagRunsForAttributionSql; the empty run times also join the capture times given toGroupIntoEvents, so an event's previous capture is right.LocalDataService.GetEmptyTraceFlagRunsAsync(feeds the tool and the UI throughGetTraceFlagChangesAsync), andAnalysisServicewith the same SQL.get_trace_flag_changes(both products) used to say that turning off the last flag "does not appear as a change here". That is no longer true, so it now says a successful empty run counts as an empty capture and no turn-off falls in this window.ConfigChangeAttributionremarks: blind spot (2) is closed.Test plan.
ViewerConfigChangesDiffTests): on then empty run is "disabled" at the run's time; on then no empty run is no change; empty then on is "enabled"; an empty run stamped at a capture that has rows is ignored and repeated empties add one turn-off.ConfigChangesReaderTests, real DuckDB): on then SUCCESS/0 run gives "DISABLED"; on then ERROR, PERMISSIONS and SUCCESS/NULL runs give no change; SUCCESS/0 run 30 h ago (before the 24 h window) then on gives "ENABLED" (baseline arm).TraceFlagEmptyRunLivePostgresTests, rig on 5684): the MCP reader, the viewer SQL and the analysis SQL return the same two times (the in-window empty run and the newest one before the window; not the ERROR, PERMISSIONS, NULL-count or other-collector rows, not the older pre-window run); the tool returns "disabled" then "enabled" newest first; on then only a failed run is no change.emptyRunTimes, the Darling live test and 2 of the 3 Lite tests failed (the no-change test passes either way, by design).*ConfigChange*,*ViewerConfigChanges*,*TraceFlag*,*CollectorRun*,*Census*,*Ratchet*, DocCommentHygiene, Startup/StorageCommandTimeout, LivePostgresCollectionHygiene, AlertReadFailureSurface, McpToolsListBudget: 1045 run, 0 failed, 3 skipped. Lite:*ConfigChange*,*TraceFlag*,*CollectorRun*, Census, Ratchet, DocCommentHygiene, McpToolsListBudget: 243, 0 failed. Builds 0 warnings. After the last merge of the shared branch I re-ran only my own classes (Darling 64, Lite 12; 0 failed).Lite parity. Checked all three Lite sites:
LocalDataService.GetTraceFlagChangesAsync(tool and UI),AnalysisService.AttributeConfigChangestrace-flag family, and theget_trace_flag_changesempty message. SQL is the same text withv_collection_log.Double-check.
trace_flagscollector runs once on connect, so empty runs are rare; a server with no flags ever has a handful of SUCCESS/0 rows, and the window query reads only those in the window plus one.LivePostgresCollectionHygieneTestsfailed once mid-lane on another lane'sWaitingTasksRollup5633Tests(nolive-postgrescollection); it was fixed on the shared branch and passed after my merge.Changelog:
SECTION: Fixed
ENTRY:
REF:
[MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704]: MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704
F5: trace-flag diff - bound the empty-run baseline at the retention, never log a failed trace_flags read SUCCESS (item 19, round 2 Medium-1, Medium-2, Low-3)
Why. Lane F4 made a successful trace_flags run with
rows_collected = 0an empty snapshot in the trace-flag change diff. Review round 2 found two holes in that. (1) Both trace_flags collectors caught everySqlExceptionand returned 0, so a denied DBCC, a timeout or a dropped connection was logged SUCCESS with 0 rows and read as "every flag turned off". (2) The baseline arm ("the newest empty run before the window") had no lower bound: an empty run older than the oldest trace_flags rows the store can still hold paired with a later capture and read as a flag turned on that had really been on for weeks. In Lite the same arm scanned the wholev_collection_loghistory on each call.What changes.
RemoteCollectorService.ServerConfig.csCollectTraceFlagsAsyncno longer has a catch. A failure reachesRunCollectorAsync, which already records PERMISSIONS (SqlServerPermissionErrors.IsPermissionDenied) or ERROR for every other collector.RunTraceFlagsTolerantAsyncis deleted and thetrace_flagsdispatch entry is the plainr.RunAsync(TraceFlagsCollector.Instance, s, ct).RunOneAsyncthen records PERMISSIONS or ERROR. DarlingWorker.cs edit (also touched by open PR PostgreSQL log text keeps row values out of the stored text, in their slot #5698 near line 10738, log masking): three hunks, all in the trace_flags collector only - the dispatch line, the deleted wrapper at the end of the file, and two doc-comment sentences that described the tolerance. No masking code touched.DBCC TRACESTATUS) is not a catch case any more:TraceFlagsCollector.AppliesToalready gates it (!target.IsAzureSqlDb) and both products record SKIPPED / skip it before the run, so collection health does not turn red there. What the old catch still covered was a permission denial, and that is the existing PERMISSIONS pattern (a yellow, deliberate least-privilege state, not a per-cycle ERROR).DarlingRetentionHorizons.FleetRetentionDaysSql/TraceFlagsBaselineFloorSqlin Storage (DailySummaryHorizon.cs). The retention is the fleet overrideconfig.config_collector_schedules.retention_daysfortrace_flagswhen set and at least 1, elseCollectorScheduleDefaults(30 days) - the same rule as the purge andDataWindowFloor. The baseline arm of all three copies (DarlingCollectorRunFactsReader.EmptyTraceFlagRunsSql,ViewerDataService.EmptyTraceFlagRunsSql,DarlingAnalysisService.EmptyTraceFlagRunsForAttributionSql) getsAND collection_time >= (window start - retention). The three constants are nowstatic readonly(interpolated); every caller (MCP tool, viewer, analysis) goes through them.RetentionService.ArchiveRetentionMonths(3), and its per-collectorretention_dayspurges nothing (grep: only the Query Store backfill reads it), so the floor iswindowStart.AddMonths(-3), bound as$4inLocalDataService.EmptyTraceFlagRunsSql(EmptyTraceFlagRunsFloor). In the SQL, so DuckDB prunes the archive by zone map. Used byGetTraceFlagChangesAsyncandAnalysisServicethrough the one shared reader.Test plan.
TraceFlagEmptyRunLivePostgresTests3/3 - newAnEmptyRunOlderThanTheRetention_IsNotABaseline_ByEveryDarlingRead_AndAFleetOverrideMovesTheBound(day -50 empty run + capture now: reader, viewer SQL, analysis SQL all empty and the tool shows no "enabled"; with a fleetretention_days = 90the same run is the baseline again);AFlagOnThenOnlyAFailedRun_IsNoChangenow plants both ERROR and PERMISSIONS.ConfigChangesReaderTestsnew...AnEmptyRunOlderThanTheRetention_IsNotABaseline...,...AnEmptyRunInsideTheRetention_StillIsTheBaseline,TheEmptyRunFloor_IsBoundInTheSql....TraceFlagsFailedRunPinTestsin both projects (Darling: plain dispatch, no tolerant wrapper, oneTraceFlagsCollector.Instancein the worker; Lite:CollectTraceFlagsAsynchas no catch and noreturn 0).RunTraceFlagsTolerantAsyncand a catch).*TraceFlag*,*Census*,*Ratchet*564/564 pass; plusDarlingWorkerTests,CollectionLogDrainForensics*,CollectorRunEmptyReasonTests,McpToolsListBudget*pass. Lite:*TraceFlag*,*ConfigChanges*,*Census*,*Ratchet*,*CollectorGateSurface*,*McpToolsListBudget*106/106. Both builds 0 warnings.LivePostgresCollectionHygieneTestsFAILS, but not on my class: it namesMcpToolAudit5633L3Tests.cs:29(lane L-a's file, nolive-postgrescollection or own-store comment). Re-run it after that lane's fix; my live class already carries[Collection("live-postgres")].Lite parity. Both collectors, both baseline SQLs and both test sets changed together. Checked: Lite
AnalysisService.ReadEmptyTraceFlagRunsAsyncgoes throughLocalDataService.ReadEmptyTraceFlagRunsAsync, so the new$4parameter reaches it with no second edit; the Darling analysis, viewer and MCP copies each carry the floor.What the seat should double-check.
DarlingWorker.csoverlap with PostgreSQL log text keeps row values out of the stored text, in their slot #5698: confirm the merge keeps both (my hunks are far from line 10738).Changelog:
SECTION: Fixed
ENTRY:
REF:
[MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704]: MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704
F6: dropped-database scope follow-ups and compose server names (round 2 Medium-3, Low-1, F2 leftover)
Why. (1)
get_query_store_clutteracceptedas_ofbut always dropped databases using today's snapshot, so an incident review of a window before a drop lost that database. (2) Ranking reads narrowed to an include list of the newestdatabase_statessnapshot, hiding a database created after that snapshot for up to 30 minutes. (3) The compose scope's exact-name test saw only enabled servers and did not trim, so an enabled server's display name could take a disabled server's name, and a padded name stopped resolving.What changes.
CurrentDatabaseScope.OmitDroppedAsynchas anasOfUtcparameter (passed toReadSnapshotAsync). The three clutter readers (GetReadCostAsync,GetPlanChurnAsync,GetConfigAsync) takeasOfUtc; the tool passes the anchor whenas_ofis set, on the per-server and the fleet-median reads. Other callers checked: FinOps index analysis and FinOps recommendations (Darling) andGetCurrentDatabasesAsyncin the FinOps recommendations (Lite) are current-state tools with noas_of, so nothing to pass.get_top_procedures_by_cpualready passed it (both products).NarrowAsyncnow EXCLUDES only true drops. NewCurrentDatabaseScope.ReadDroppedAsyncruns one grouped read ofdatabase_states(MAX(collection_time)per database since a floor); a name whose newest stamp is older than the newest overall is dropped. Floor = the window start, never nearer than the 6-hour horizon and never further than 7 days (DropLookBack: a drop only matters if it falls inside the ranking window; 7 days bounds the scan to about 336 snapshots). A name no snapshot lists is never excluded. A newest snapshot older than 6 hours is unknown (nothing excluded). No fact-table probe.DatabaseFiltergets the smallest exclusion form:Excluding(...),ExcludedNames,ExcludeParameter(),ExcludeClause(column, index)=($n::text[] IS NULL OR col IS NULL OR col <> ALL($n))(NULL-safe on the column). The filter staysIsAll, so a reader that does not apply it reads every database as before. Readers checked: only the top-procedures path takes a narrowed filter (the singleNarrowAsynccaller).TopProceduresSql(both passes) andTopProceduresHourlySqlcarry the new$6line; the hourly ceiling moved from$6to$7; both bind sites addExcludeParameter(). The existing$5list predicate text is unchanged. The viewer has its own copy of the statement and never narrows.ComposeServerScopeloads EVERY registered server (LoadAllServersSql, nois_enabledfilter), trims the name, keeps an exact storedserver_name(any registration) before trying display names, then resolves as before (the resolver already prefers a stored-name match over a display-name match, case-insensitively). A name with no exact match stays as written (raw), so removed servers keep the "left as written" path.QueryStoreBackfillState.LastSeenPerDatabase*Sql(shared text),GetDroppedDatabasesAsync(same rules),GetTopProceduresByCpuAsync(excludeDatabaseNames:)withBuildDbExcludeClause;get_top_procedures_by_cpuuses them. Lite has no clutter tool, so no twin for Medium-3.Test plan.
Darling.TestsandLite.Tests: 0 errors; 0 new warnings (the one warning, xUnit2013 inDarlingOperatorGroupScriptTests.cs, is another lane's file).asOfUtcgave 7 failures (padded names x3, stored-beats-display, the registry-read pin, the live disabled-server test, the clutter as_of test); restored, they pass.QueryStoreClutter_WithAsOf_KeepsADatabaseDroppedAfterTheWindow,DroppedFrom_NamesOnlyWhatAnEarlierSnapshotListedAndTheNewestDoesNot, NewDb-ranks assertions added toTopProcedures_AndTheClutterReads_...(heaviest, in no snapshot, takes top 1), compose padded / stored-beats-display / registry-read pin / liveResolveNamesAsync_SeesDisabledRegistrations; LiteDropped_IsWhatAnEarlierSnapshotListedAndTheNewestDoesNot,TopProcedures_LeaveOutADroppedDatabase_AndRankADatabaseNoSnapshotNames.Darling.Tests(rig port 5686, UTC): all*Census*,*Ratchet*,LivePostgresCollectionHygieneTests, my classes, DatabaseFilter and top-procedure SQL pin classes: 833 run, 3 failed. (a)ComposeServerScopeResolveNamesTests.ResolveNamesAsync_SeesDisabledRegistrations: connect timeout under the heavy parallel run; passes alone after the run. (b)ObjectStatsDatabaseFilterLiveTests...JSON parse failure under the same load; passes alone. (c)LivePostgresCollectionHygieneTests: missing own-store comment on my class (fixed and re-run) and on another lane'sMcpToolAudit5633L3Tests, which still fails: that class is not mine.Lite.TestsCensus, Ratchet,CurrentDatabasesTests,DatabaseFilterTests,QueryStatsCpuRankingReaderTests: 81 run, 0 failed.origin/fix/5633-mcp-tool-auditcommits (Lane L3'sMcpToolAudit5633L3LivePostgresTestsand others) was pushed without a rebuild; the rig was stopped before the 60-minute wall.get_top_procedures_by_cpuagainst a fleet-sizedatabase_states(about 336 snapshots x many databases at the 7-day cap) was not timed.Lite parity. Checked: the only Lite use of the current-database set was
get_top_procedures_by_cpu(replaced) and FinOps recommendations (post-read, current state, kept). Litev_database_statesreads use the same shared SQL text.What the seat should double-check.
McpToolAudit5633L3Tests(lane L3) lacks the#1776 own-storecomment; hygiene fails on it.DropLookBackand the window-start floor are my call (the ruling allowed "a bounded look-back you justify").OmitDroppedAsync(post-read, used by the clutter tool and FinOps) still judges by the newest snapshot only, as before: a database created after it is kept by the row-time rule, not by "never seen". The ruling covered the Narrow reads only.McpToolsListBudgetTests/CrossAppMcpToolInventoryPinTests: no tool text or inventory changed here.Changelog:
SECTION: Fixed
ENTRY:
REF:
[MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704]: MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704
F7: CI guard failure, distinct high-DOP count, Lite waiting-task tie order, Darling CPU window behaviour test
Why. Review round 2 findings Medium-4, Low-2, Low-4, plus the red
Guard testsjob (run 38308174340):LivePostgresCollectionHygieneTests.EveryClassUsingTheSharedStore_IsSerializedOrDocumentsWhyNotlistedMcpToolAudit5633L3Tests, whose two tests drive tools against the shared PostgreSQL store.What changes.
AuditConfig_EchoesTheStoredEditionString_...,CompareAnalysis_RefusesOverlap_...) moved unchanged intoDarling/Darling.Tests/McpToolAudit5633L3LivePostgresTests.cs, a new class with[Collection("live-postgres")]and the existingLiveStoreCleanup.RunAsynccleanup. The pure tests stay inMcpToolAudit5633L3Testswith no collection.high_dop_queriesis nowCOUNT(DISTINCT CASE WHEN <same DOP/MAXDOP predicate> THEN COALESCE(database_name,'') || '/' || COALESCE(query_hash,'') END)inPgFactCollector.QueryPerf.csandDuckDbFactCollector.QueryPerf.cs, the same key and NULL-hash rule asspilling_queries(a NULL hash collapses to one per database). Bands decision: the 5/20 bands stay. The fact's text says "N queries", and the count now is literally queries, so the bands now mean what they say; a server that used to hit full severity from one query seen in 20 snapshots now needs 20 distinct queries. The stale-MAXDOP cross-check parity pins (QueryHighDopStaleMaxDopParityTests, both projects) keyed onCOUNT(CASE ... THEN 1 END); I updated their canonical string, census regex and guard regex to the DISTINCT form (the guard regex now requires DISTINCT, so reverting to a row count fails the pin). The unguarded-count negative sample still lands in the census and misses the guard.arg_maxstruct keys with DuckDB ordered aggregates using the PostgreSQL ordering exactly: blockerfirst(CASE WHEN blocker <> session THEN blocker END ORDER BY wait DESC NULLS LAST, blocker ASC NULLS LAST), fallbackfirst(blocker ORDER BY wait DESC NULLS LAST, blocker ASC NULLS LAST) FILTER (WHERE blocker <> session), resourcefirst(resource ORDER BY wait DESC NULLS LAST, blocker ASC NULLS LAST, resource DESC NULLS LAST). Darling's SQL is unchanged.FinOpsCpuRightSizingWindowLiveTests(Darling, own scratch database) runsDarlingFinOpsRecommendationsReader.GetRecommendationsAsyncon seeded data. Test 1: 32 cores, last 24 hours at 8%, a busy 90% stretch four days back; the CPU row must read "CPU: reduce from 32 ... (P95 CPU 8.0%)" and the detail must start "From the last 24 hours, P95 CPU utilization was 8.0%". Test 2: P95 8% but one 60% spike, so the Utilization verdict is RIGHT_SIZED; neither CPU row may appear. The source pin inRightSizingWindowTestsstays.Test plan.
WaitingBlockedDatabaseFilterToolTests.WaitingTasks_TiedAndNullWaitThreads_PickTheSameRowAsDarling, DarlingWaitingTasksRollup5633Tests.TiedAndNullWaitThreads_PickTheSameRowAsLite). Session 80: self-blocked longest thread, tie at 300 ms between blockers 50 and 40 gives blocker 40; session 81: the only other-blocked thread has a NULL wait and still supplies blocker 70; session 82: equal waits give blocker 22, resource of blocker 22.arg_maxSQL fails the tie/NULL test and old row count failsHighDopQueryCount_CountsDistinctQueries_NotSnapshotRows; Darling old row count fails itsHighDopQueryCount...live test; a 7-day utilization window (AddDays(-7)) fails the busy-cluster CPU test. The Darling tie/NULL test passes on both old and new Darling code by design (it pins parity with Lite; Darling's SQL was already the reference).-trait Stage=Guard: 1588 total, 0 failed (this includesLivePostgresCollectionHygieneTests).*Census*,*Ratchet*,*LivePostgresCollectionHygiene*,*QueryHighDop*,*QuerySpillsDistinct*,*WaitingTasksRollup5633*,*FinOpsCpuRightSizing*,*McpToolAudit5633*,*RightSizingWindow*,*McpToolsListBudget*: 635 total, 0 failed.-trait Stage=Guard -failSkips: 496 total, 0 failed. Lite Census, Ratchet, QueryHighDop, QuerySpillsDistinct, WaitingBlockedDatabaseFilter, McpToolAudit5633, McpToolsListBudget, FinOpsTests: 177 total, 0 failed.DarlingOperatorGroupScriptTests.cs, from the dev merge, not this lane).-failSkips(CI's exact condition; the run above had the rig set), the full suites, a mutation proof that the RIGHT_SIZED test fails if the CPU rows ignore the verdict.Lite parity. Medium-4 and Low-2 changed in both products with the same tests; the Low-4 behaviour test already existed in Lite (
FinOpsTests.VmRightSizing_BusyClusterFourDaysAgo...), this adds the Darling one. The parity pins were updated in both projects together.What the seat should double-check.
QueryHighDopStaleMaxDopParityTestsedits in both projects (canonical string, census regex, guard regex): I changed pins deliberately because the guarded count shape changed; the guard regex is stricter than before (requires DISTINCT and the key).Changelog:
SECTION: Fixed
ENTRY:
REF:
[MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704]: MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704
F8: fix lane (wave 2 review: collection caveats are written by the scheduled pass only)
This section replaces W2A's change. W2A (commit 2a688bf) granted the mcp role INSERT, DELETE and a column UPDATE on
collect.analysis_collection_caveats, plus a "caveats NOT saved" sentence inpersistence_note. The seat ruled against it (least privilege). Ignore the W2A section above for the grants, theboolreturn,LastCollectionCaveatWriteFailedand thepersistence_notetext: all of that is gone. The ruling, in two sentences:collect.analysis_collection_caveatsis the scheduled analysis pass's state, so only the scheduled pass (the worker, running as the service role) writes it. Every on-demand pass (MCPanalyze_server, with or withoutas_of) computes its caveats for its own response and the in-process ledger and never writes the table.Why. Review findings W2A-1 to W2A-6 (
handoffs/h6/review-w2.md): anas_ofrun over a past window could replace the standing "current" rows (W2A-1); any MCP window choice could clear or forge them (W2A-2, W2A-3); the grant comments overstated their protection (W2A-3); the role docs, the privilege census and theApplyPassAsyncdoc were wrong or thin (W2A-4 to W2A-6). Withdrawing the write answers all of them. Plus one CI failure: run 38315771866,TsqlConventionGuardTests.TheMemberScan_ReadsEveryDeclarationWhole(CurrentDatabaseScope.cs Narrow).What changes.
DarlingManagedRoles.csandDarling/tools/provision-roles.sql: W2A's two GRANT statements and its doc-comment edits are reverted, so the comments say "exactly two narrow analysis INSERTs" again. Nothing else in provisioning changed. The role SQL that runs on every start never granted this table to mcp on dev (git grepof origin/dev finds noanalysis_collection_caveatsin either file), so there is nothing to revoke on a released store.DarlingAnalysisService.AnalyzeAsync(both overloads) takesbool persistCollectionCaveats = false; the caveat write isif (persistCollectionCaveats). There was no scheduled-versus-on-demand flag to reuse (AnalysisContext.PersistFindingsonly means "not anchored", and unanchored MCP runs do persist findings), so this is the one new parameter. It defaults to false so a new on-demand caller cannot write the table by forgetting a flag.DarlingWorker.cspassestrue(one call site, one line plus a comment; the only otherAnalyzeAsynccaller in the product is the MCP tool). No web-triggered analysis exists.ApplyPassAsyncreturnsTaskagain (the worker has no use for a bool). Its doc is fixed (W2A-6): it always opens a connection and always runs the DELETE; the upsert is skipped when nothing is unread. It now says scheduled passes only.DarlingMcpTools.cs: theCollectionCaveatsNotSavedNoteconstant,ComposePersistenceNoteand the call are removed.persistence_noteis back to its dev shape, since an on-demand pass never tries to write.TsqlConventionGuardTests:Darling/PerformanceMonitor.Darling.Storage/CurrentDatabaseScope.cs Narrowadded toKnownTruncatedRanges.Narrowisfilter.IsAll && dropped is { Count: > 0 } ? ... : filter, the An RDS or Aurora target can name the AWS role Darling assumes for it, so one host can monitor clusters in several AWS accounts #5452 property-pattern shape: the brace walk closes the range at the pattern's brace and strands only the ternary arms, which hold no string literal. I checked that the file's SQL sits in members the walk reads whole, so no census counts a stranded site.AnalyzeAsync(AnalysisContext context)now match the prefix (CollectionCaveatsParityTests,AnalysisWindowEmptyRuleParityTests,DarlingAnalysisServiceEngineRoutingTests), andDarlingAnalysisPipelineTestslooks the overload up with the extrabool.Privileges the mcp role now holds on
collecttables (census on a real role, real managed batch): SELECT on every table (schema-wide grant plus the default privilege for later tables), and INSERT onanalysis_findings. Nothing else: no INSERT, UPDATE, DELETE, TRUNCATE, TRIGGER, REFERENCES (table or column level), no MAINTAIN (checked when the server has it), no sequence USAGE/UPDATE. Onanalysis_collection_caveatsit holds SELECT only (the web panel andget_collection_healthread it).Test plan.
McpCollectionCaveatGrantLiveTestsrewritten (3 live tests, real managed batch on a scratch store): (1) the strict census above, asserting the mcp role holds NO write on the caveat table andanalysis_findings:INSERTis its only collect write (W2A-5: TRIGGER, REFERENCES, MAINTAIN where present, sequences); (2)analyze_serveras mcp, with and withoutas_of, leaves three seeded caveat rows (two for the server, one for another) exactly unchanged, with the old grants put back so the privilege is not what protects them; (3) on the owner pool, the default pass and the anchored pass leave the table unchanged and a pass withpersistCollectionCaveats: true(the worker's) still clears the stale row.DarlingManagedRolesTests.McpRole_HoldsNoWriteOnTheCaveatTable_InTheManagedBatchOrTheByoScript(no database): no non-comment statement in the managed batch orprovision-roles.sqlnames the table;GRANT ALL ... TO mcpand a collect-wide non-SELECT grant are caught (W2A-5's regex gap; the test also asserts the patterns match sampleGRANT ALL ON collect.analysis_collection_caveats TO mcp;shapes).DarlingSecuritySplitLiveTestsnow asserts mcp lacks INSERT, UPDATE, DELETE, TRUNCATE, TRIGGER and REFERENCES on the table.|| true), all 3 live tests and the unit test failed (4 of 4); restored, they pass.*Census*,*Ratchet*,*Role*,*Grant*,*CollectionCaveat*,*AnalysisAsOfAnchor*,*TsqlConventionGuardTests*,*LivePostgresCollectionHygieneTests*,*AnalyzeServer*,*DarlingAnalysisBudget*: 1082 run, 1 failed (CollectionCaveatsParityTests, a source pin on the old signature, fixed above), 17 skipped (rig has noDARLING_TEST_PGRUNTIME). Then*Parity*,*EngineRouting*,*DarlingAnalysis*plus my classes: 188 run, 1 failed (DarlingAnalysisPipelineTestsreflection pin, fixed above). Final re-run of the fixed classes: 26 passed, 0 failed.TsqlConventionGuardTestspassed (the CI failure is fixed). Build: 0 warnings, 0 errors, after merging origin/dev.Installer.Tests, the runtime-bound role tests that needDARLING_TEST_PGRUNTIME.Lite parity. Lite has no store roles (DuckDB, no GRANTs) and no collection-caveat table; Lite's
AnalyzeAsync(AnalysisContext)is untouched (the shared parity pins match by prefix, so they still read both products). Nothing to mirror.What the seat should double-check. (1) The W2A section above is now obsolete; its Changelog entry should be dropped in favour of the one below. (2) After this change an MCP
analyze_serverrun never updates the caveat panel; only the next scheduled pass does (5-360 min, or never ifAnalysis.Enabledis false, then the 7-day prune). (3) The one-lineDarlingWorker.csedit sits in a file other PRs touch; keep both sides on a conflict.Changelog:
SECTION: Fixed
ENTRY:
analyze_serverthrough MCP no longer tries to write the collection-caveat table, so its permission-denied error is gone ([MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704]) - Everyanalyze_servercall through the MCP server also tried to save the list of data families the pass could not read, and the MCP role is not allowed to write that table, so each call logged a permission-denied error. That table is the scheduled analysis pass's state, so only the scheduled pass writes it now. MCP runs, with or withoutas_of, still show their own caveats in the response and no longer replace the saved ones. The MCP role gets no new permission and no database change is needed.REF:
[MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704]: MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704
F9: size the background workers by host RAM (item 14, wave 2 review findings W2B-1 to W2B-4, plus the README pin)
Why
Lane W2B raised
timescaledb.max_background_workersto2 x hypertables + 16(160) for every host. The wave 2 review (W2B-1) estimated that 139 concurrent job backends can exceed RAM on a 4 GB host, and found the formula keyed on the wrong count (W2B-4), a v8 legacy fallback that accepted each worker line alone (W2B-3), and two comments that disagreed with the facts (W2B-2, W2B-4). CI run 38315771866 also failedReadmeDerivedCountPinTestsbecause the pin derived 74 while the README said 160.Measurement (rig on port 5684, PostgreSQL 18 with the bundled TimescaleDB, a store built by the real migrations plus the job-creating convergence steps)
The store held 142 jobs (94 compression, 23 refresh, 23 retention, 2 system). With every scheduled job set overdue (
alter_job(..., next_start => now() - 1 minute)), the rig was restarted at each sizing while a sampler summedPrivateMemorySize64of the postgres processes whose executable is under the rig directory, every 250 ms for 45 s (script kept out of the repo):out of background workerslinesReading it honestly:
maintenance_work_mem(1 to 1.5 GB) and a refresh sorts atwork_mem, which this does not measure. So I did not raise the factor on the strength of the small number.What changes
DarlingManagedPostgres.DeriveWorkerSettings(hypertables, quantizedRamBytes):workers = clamp(16 x RAM GB, hypertables + 2, ceiling),max_worker_processes = 3 + workers + 8(unchanged). RAM is the same whole-GB figure the v8 fingerprint already records andDeriveMemorySettingsuses (QuantizeRam), so no new input and no fingerprint change. The v8 block,DarlingStoreHostProfile(--check-settings) and the every-start v8 heal log all pass it.DeriveWorkerFloor= hypertables + 2 (74 / 85), the shape of every release up to 3.10.0. The v2 block (no RAM input) writes the floor; v8 is later in the file and wins, as the review said.DeriveWorkerCeiling= hypertables + 3 x continuous aggregates + 2 + 16 = 177 at 72 hypertables and 29 aggregates (W2B-4: counts hypertables AND aggregates and every policy kind; the real store has 142 jobs, the ceiling covers 143 with room). NewTimescaleSupport.ContinuousAggregateCount(the distinct views of the hourly, daily, baseline, off-grid and frozen tiers); constantsWorkerSlotsPerGb,PolicyJobsPerContinuousAggregate,NonObjectPolicyJobsreplacePolicyJobsPerHypertable.ManagedConfMigration.cs(W2B-3): the v8 legacy fallback accepts a worker line only when BOTH assignments in the same span equal the legacy PAIR (hypertables + 2, + 13), read the way v2 pairs them. TherebuiltCurrentandrebuiltLegacychecks in the v2 classifier were dead (the earlierM == N + 11test already implies both) and are removed with a comment saying why.mem_limitunder about 4 GB has no headroom for both; the two comments on an idle slot's cost now say about 25-40 KB of shared memory and no process, and that a slot a restart FILLS is a backend.docker-compose.ymlgoes back to the floor, 74 / 85: a container has no host-RAM figure and a 2 GB container is at the floor of the rule anyway; the comment says amem_limitof 12 GB or more may use 177 / 188. The README worker-sizing section states the rule, the ceiling and an 8 GB example (128 / 139) with the 4 GB and 12 GB values; the runbook carries the same example. The README process-count FAQ no longer says "hypertable count + 2 (about 57)".ReadmeDerivedCountPinTestsderived its expected pair ashypertables + 2while the README (W2B) said 160 / 171. The pin now derives the 8 GB example fromDeriveWorkerSettingsand matches the sentence "For an 8 GB host that is 128 and 139 for 72 hypertables (the 71 collector tables plus", so the README and the pin agree and cannot go stale on other hosts.PgLogEventsPipelineTests(runbook and README sentences) andCiComposeWorkerSizingTests(floor) follow.Test plan
JobCountWithinWorkerSizingTests(2): the pure pin at 4 / 8 / 16 GB, the empty store, one more aggregate, and the clamp bounds at 1 GB to 1 TB; the live fact now asserts the formula's aggregate and hypertable counts equal the migrated store's real ones, that the ceiling covers the real 142 jobs + 1 scheduler and sits within 40 slots of them, and that a 16 GB host gets a worker per job.ManagedConfMigrationTests:ClassifyLines_V8Block_With74And171_IsNotServiceOwned(both lines HandEdit) andClassifyLines_V8Block_WithTheLegacyWorkerPair_IsOurs;HardwareSizingConfAppend_WorkerCountsAreTheRamSizedPair_NeverBelowTheV2Floor(4 / 8 / 16 / 32 GB, v2 floor vs v8).DARLING_TEST_PGandDARLING_TEST_PGRUNTIMEset): JobCountWithinWorkerSizingTests 2/0 failed; ReadmeDerivedCountPinTests 5/0; CiComposeWorkerSizingTests 4/0; PgLogEventsPipelineTests 94/0; CiClusterWorkerSizing (both) and every ManagedConf class together 245 total, 0 failed, 1 skipped (includes the rehearsal fixture's legacy v2 and v8 blocks); every*Census*,*Ratchet*,*DocCommentHygiene*class 638 total, 0 failed. DarlingManagedPostgresTests (full class): 231 total, 1 failed (HardwareSizingConfAppend_EmitsHostDerivedSettingsstill pinned W2B's 96 / 107 at 40 hypertables; now 145 / 156, the 32 GB host at the ceiling), 3 skipped; after the fix andgit merge origin/devthe HardwareSizingConfAppend and WorkerSizingConfAppend methods (14) pass, and the other classes above re-ran green on the merged tree: 808 total, 0 failed. The full class was not re-run after the merge.ClassifyLines_V8Block_With74And171_IsNotServiceOwnedfails; restored.What the seat should double-check
darling-pgcluster still runs 74 / 85 (workflow untouched, per the brief).Changelog:
SECTION: Fixed
ENTRY:
REF:
[MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704]: MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704
F10: Darling CI pin failures from run 38321499493 (head 4535bb3)
Why
CI run 38321499493 failed 7 Darling tests (plus 2 in shard 5 found when its log finished). Every one is a pin or guard that reads source an earlier lane on this branch changed; the lanes ran only their own classes plus Census and Ratchet. All nine are test-side fixes: in each case the product code is right and the pin's literal text no longer described the right shape, and each pin was updated to the new shape (not loosened) with the reason in a comment. No product code changed in this part.
What changes (per failure: cause commit, code or pin, fix)
audit_config). Cause: 894159d (item 10, L3) made audit_config callGetLatestServerPropertiesAsyncto echo the stored Edition string. Code right: the payload is the analysis pass's facts plus an edition label, not a latest snapshot, so it publishes nocaptured_atand stamping it would imply a snapshot time the facts do not have. It is the same case as the two CPU rankings, which read the newest server_properties row as an input. Fix:audit_configjoinsLatestLookupInsideAnotherRead(the list's own control fails if the tool ever gainscaptured_at) and the doc comment names it.asOfUtc: asOfUtcto the fleet reads. Code right and better: the fleet read keeps the samerequestedStart/nowwindow as the tool's own read and now also the same as_of anchor for dropped-database handling, so the fleet median and the server answer see the same databases. Fix: the pin matchesGetReadCostAsync(postgres, fleetIds, requestedStart, now, asOfUtc: asOfUtc, cancellationToken: cancellationToken)and the churn twin, so dropping the anchor from a fleet read now fails.is_percent_growth, growth_pctafterfile_id. I checked the reader's ordinals first:GetLatestDatabaseSizesAsyncmaps file_id at 11, is_percent_growth at 12, growth_pct at 13, matching the SQL order, so no data bug. Fix: the regex pins the full tailvolume_free_mb, file_id, is_percent_growth, CAST(growth_pct ...) AS growth_pct FROM v_database_size_stats, so a column slipped in before file_id still fails.ProvisioningStatus == NotApplicabletest with one shared predicateCpuVerdictAllowsDownsizing, true only for OVER_PROVISIONED, so NotApplicable (and RIGHT_SIZED, and no-sample) stand down by construction. Code right: both CPU rows use it (the compute row throughCpuRightSizingTargetCores, the prescriptive row inDarlingFinOpsRecommendationsReader). Fix: the pin asserts the predicate is defined on OverProvisioned alone, the compute row gates on it, the reader's prescriptive row gates on it, and no inverted test exists. The sibling behavior test (NotApplicable returns null, OverProvisioned returns a row) still executes the stand-down and passes.get_trace_flag_changes, Cause: item 19 (9d195d6, 5f794e5).WebDataStartNoteConfigAndLogTests.TheAdmittedReads_AnswerTheirWord_...: the source-text check looked forNoChanges(in the tool body; the empty path now goes throughNoTraceFlagChanges(, which returnsNoChangesunless a successful empty run says no flag is on (then statusemptyitself). Pin now acceptsNoTraceFlagChanges(too.DarlingMcpConfigHistoryToolsLivePostgresTests.APreWindowSnapshot_IsNotCovered_...(KeyNotFoundException onhints): its seed logs a SUCCESS run withrows_collected = 0for trace_flags. Since item 19 that is an empty snapshot, which turns the seeded flag off inside the window, so the tool correctly answers with that change (top-leveleffective_start, nohints). Code right. The case is about coverage, so the seed's second run now stores one row (runRows: 1, a new optional seed argument, default 0 so the other cases are unchanged). Reproduced on my rig before the fix and green after.Test plan
McpLatestSnapshotStampTests,QueryStoreClutterTests,AzureSiblingSizePayloadTests,NoServiceObjectiveToResizeTests,WebDataStartNoteConfigAndLogTests,DarlingMcpConfigHistoryToolsLivePostgresTests..csleaf, plus every file naming a rarely used basename; 13 very common basenames such as McpHelpers, DarlingWorker and DarlingMcpTools matched by leaf path only): 581 test files, 703 classes, run in 3 chunks on the rig against one database. 10,377 tests, 7 failed, 44 skipped. The 7 were all live classes (FinOpsRecommendationsSecondaryReplicaLiveTests, QueryStoreClutterLivePostgresTests, FleetPartialDatabaseFailureBandTests, QueryStoreBackfillSkipFailingDatabaseTests, FinOpsStorageGrowthGoldenLiveTests, FinOpsRecommendationsToolParityLiveTests, GatedCollectorRowsLiveTests); re-run together on a freshly created database they passed (53 tests, 0 failed), so the failures were shared-database interference from running 700 classes in one database, not code. The sweep found no new pin failure beyond the nine listed above.Lite parity
Test-only change on the Darling side. F11 owns the Lite half of the same CI run (ArchivableTableBareRead, AsOfWindowAnchor on Lite/Mcp, McpWindowNotice trace flags). Lite.Tests was not touched here. The Darling-directory leg of
AsOfWindowAnchorTestspassed in that CI run and is in the sweep.What the seat should double-check
audit_configis listed as "latest lookup inside another read" rather than stamped. If the seat prefers acaptured_aton audit_config (the properties row's own collection time), that is a product change, not a pin edit.Changelog: none (test or internal only)
F11: Lite-side CI failures on run 38321499493 (3 tests)
Why. Three Lite-side tests failed on head 4535bb3. Each reads source or seeds data that an earlier lane on this branch changed, and those lanes ran only their own classes.
What changes, per failure
ArchivableTableBareReadSweepTests.EveryBareReadOfAnArchivableTable_IsOnTheBareOnPurposeList(LocalDataService.CurrentDatabases.cs, 2 bare reads at lines 43 and 122). Cause: 6cfa952 (item 4), kept by 3b779b1 and 59feb9b. The code was right and the sweep was seeing a false positive. Both reads already runv_database_states(hot plus archive): the shared backfill SQL namesdatabase_states, and the call site swaps it with.Replace("FROM database_states", "FROM v_database_states"). The sweep matchesFROM database_statesin that Replace's first literal. Fix: oneThroughArchiveViewhelper whose match text isdatabase_states WHERE, so no bare FROM literal is left. No allowance was added: the reads are not bare, and the 6 h to 7 d look-back can reach archived rows, which is why the view is right.AsOfWindowAnchorTests.EveryToolThatTakesTheAnchor_ActuallyUsesIt(Lite/Mcp). Cause: 9d195d6 (L2 items 3 and 19). The pin was right, and the code was wrong in two ways.get_trace_flag_changescalledGetCollectorRunFactsAsyncwith no anchor at all.get_top_procedures_by_cpupassedwindowEndpositionally, which did anchor the window count. But the newest-run read insideGetCollectorRunFactsAsynchad no time bound in either tool.as_ofcall before a collector's first run read "the newest successful run ever" and gave an empty reason as of now. For trace flags that was "No trace flags are enabled" from a run that had not happened yet.asOfUtc: windowEnd.GetCollectorRunFactsAsyncbounds the newest-run read at the anchor (newCollectorLatestSuccessAtOrBeforeSql,collection_time <= $4).DarlingCollectorRunFactsReader.ReadAsyncbounds the newest run atwindowEndUtcwhen it is given (newLatestSuccessAtOrBeforeSql). The trace-flag changes tool now passes its window end. This is in F10's side of the tree, but the files (DarlingCollectorRunFactsReader.cs, one line in DarlingMcpConfigHistoryTools.cs) are not on F10's list. The Darling AsOfWindowAnchorTests case passed before my change because it only checksdataService.calls.McpWindowNoticeConfigAndLogToolTests.AnEmptyWindow_PastCoverage_CarriesTheFloorAndTheNote_UnderHints(TraceFlags)(KeyNotFoundException onstatus). Cause: 5f794e5 (item 19, trace-flag diff reads zero-row runs as empty snapshots). The code was right and the test seed was wrong. The test seeds flag 1117 and then 28 h of SUCCESS runs withrows_collected = 0. Under item 19 that is the flag turned off, a real change, so the tool correctly returned a change payload, not the empty envelope. The shared window-notice shape is intact (the payload still carries it). Fix:SeedCoverageAsyncandSeedLogRunsAsynctake arowsCollectedargument, and this one test's trace-flag runs store one row (the collector found the same flag each time). Nothing else changed.Test plan
*Census*and*Ratchet*class): Total 2425, Failed 0.AsOfWindowAnchorTests,CollectorRunEmptyReasonTests,*Census*,*Ratchet*,*McpToolsListBudget*,*McpConfigHistory*: Total 649, Failed 0, Skipped 8 (live, no rig). Both products' anchor cases pass.TraceFlagChanges_AsOfBeforeTheOnlyRun_AnswersAsOfThen_NotAsOfNow(fails with the trace-flag tool call reverted, proven once) andCollectorRunFacts_WithAnAnchor_IgnoreRunsAfterIt; Darling SQL pin forLatestSuccessAtOrBeforeSql.Lite parity. Items 2 and 3 are Lite and Darling both; the Darling twin of item 2 is fixed (above). Item 1 is Lite only (Darling has no such sweep).
What the seat should double-check
as_of, the newest-run read is now bounded at the call'snow. A run committed after the call started is not seen, which is the intended anchor semantics.Changelog:
SECTION: Fixed
ENTRY:
REF:
[MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704]: MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704
F12: top procedures (and two sibling reads) say which databases they left out; the silent catch now logs
Why. claude[bot] finding D: with no
database_name,get_top_procedures_by_cpunarrows the read to "databases the server still has" (#5633 item 4). If the only activity was in a database dropped since, the empty page said "the procedure_stats collector ran ... and recorded nothing for it", which is false (it recorded rows there) and pointed at a filter the caller never made. Second finding:CurrentDatabaseScopehad twocatch ... { return null; }blocks that dropped the exception, so a SQL or ordinal mistake would silently turn all of item 4 off.What changes
PerformanceMonitor.Common/CollectorRunFacts.cs, newDatabasesLeftOut.cs,McpHelpers.WithDatabasesLeftOut):databases_left_out(sorted names, first 20) anddatabases_left_out_note(says the newest database snapshot no longer lists them, dropped or detached since; the full count when over 20; "name one in database_name to read its history"). Both keys ride an empty page and a page with rows. A page that left nothing out has the keys null on the data page and absent on an empty page.{"status":"empty","message":"No procedure ran in the last 168h in a database this server still has: ... 1 database(s) the server no longer lists were left out of this read (see databases_left_out).","databases_left_out":["GoneDb"],"databases_left_out_note":"These databases were left out of this read because the server's newest database snapshot no longer lists them (dropped or detached since an earlier snapshot did). Name one in database_name to read its history."}OmitDroppedAsyncnow reports the omitted names (omittedDatabases); the tool names them in both shapes, and when only omitted databases had rows the miss isempty("No Query Store rows ... for a database this server still has ...", database echo kept).IndexAnalysisWithSnapshotTimes.OmittedDatabases; the tool adds the envelope keys and the empty sentence claims only the databases still present.CurrentDatabaseScopemethods take an optionalILogger? logger(before the CancellationToken, CA1068) and the two catches now calllogger?.LogWarning(...)with the failing read, server id and exception message, once per occurrence, the pattern the rest of Darling.Storage uses (CollectionCaveatStore,DarlingModuleMap). Fail-open kept. The logger is threaded fromget_top_procedures_by_cpuandget_query_store_clutter(both now take the DIILogger, already an allowed seat). Not threaded: FinOps index analysis and recommendations, whose tools have no logger parameter, so a failure there is still not logged (the parameter exists; threading it needs a DI parameter on those tools). Lite's twin catches (LocalDataService.CurrentDatabases.cs) nowAppLogger.Warn.WebTopProceduresColumnsTests(the tool signature now hasILogger? logger = nullbetweendetailand the token),EmptyAnswerDatabaseEchoTests(clutter has a thirdStatusForDatabaseempty answer, count 2 to 3).Test plan
DroppedDatabaseScopeLiveTests.TopProcedures_SaysWhichDatabasesItLeftOut_...on a rig,LiteMcpServerClockTests.TopProcedures_SaysWhichDatabasesItLeftOut_...): GoneDb has rows, LiveDb none, newest snapshot names LiveDb. Asserts the sentence, the names, the page with rows, the named-database read (nothing left out), and for Darling the clutter empty page. Both fail on the old shape (reverted product file once on each side:Assert.DoesNotContain() Failure).CollectorRunEmptyReasonTests(new words, unavailable branches unchanged, cap at 20 with full count),FinOpsIndexAnalysisViewTests(empty sentence plus names).McpToolsListBudgetTests): Darling 111 classes, 1965 tests, 2 failures, both stale pins from my change, fixed (above), re-run green; after merging the branch again, 643 targeted tests green. Lite 28 classes plus Census/Ratchet, 761 tests green; post-merge Lite 82 green. Builds 0 warnings.Lite parity. Top procedures done and tested; Lite shares
CollectorRunFacts,DatabasesLeftOutandMcpHelpers.WithDatabasesLeftOutwith Darling, so the words are identical. No tool description changed (budget pins untouched).Double-check.
get_top_procedures_by_cpuandget_query_store_cluttergained anILogger? loggerDI parameter (schema unchanged; census green).OmitDroppedAsyncand the three clutter readers gained an optionalomittedDatabasescollector beforelogger. The index analysis and recommendations reads still fail open without logging (see above).Changelog:
SECTION: Fixed
ENTRY:
databases_left_out, with a note) and the empty sentence covers only the databases the server still has. The Query Store clutter and index analysis reads do the same, and the dropped-database lookup now logs a warning when it fails open instead of failing silently.REF:
[MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704]: MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704
F13: bot-review smaller findings and stale PR sections
Why. The claude[bot] review of 168d822 left four smaller findings plus two PR sections that F4 and F1 had made stale. Pushed at 0095611.
What changes.
DarlingSessionReader.WaitingTasksSqlnow groups and projectsrtrim(wait_type)(it grouped on the raw column). Rows written before Wait names lose their trailing space, and two Hyperscale timer waits are ignored #4884 carry a trailing space, so a session's padded and trimmed rows used to split into two rows and the padded name was shown. Lite andViewerDataService.BlockingTrendsalready did this. The GROUP BY repeats thertrim(...)expression because a barewait_typein PostgreSQL would bind to the input column. Lite's DuckDB fixture the bot asked for already exists (WaitingBlockedDatabaseFilterToolTests.WaitingTasks_TiedAndNullWaitThreads_PickTheSameRowAsDarling: self-blocked longest thread, tied other-session blockers expect the lower id, NULL-duration blocker still reported), so I added nothing there.DarlingRetentionHorizons.EmptyTraceFlagRunsSql(Darling.Storage, inDailySummaryHorizon.csbesideTraceFlagsBaselineFloorSql, which it calls). It isstatic readonly, notconst, because the floor is built by a method call.DarlingCollectorRunFactsReaderandViewerDataService.ConfigChanges.cslost their byte-identical copies and reference it.TraceFlagEmptyRunLivePostgresTestsnow compares the shared text against the analysis copy instead of the viewer copy.DarlingAnalysisService.EmptyTraceFlagRunsForAttributionSqlstays separate (it reads the basecollection_log, notv_collection_log); its doc now says why. Lite's DuckDB copy is untouched except for a doc cref.FinOpsRecommendationFigures.VmRightSizingandVmCpuTargetCores: the CPU parameter isp95Cpu24h(the caller already named its local that). The memory parameter keeps its 7-day name.Lite/ServicesandDarling.Storage/FinOps). L5's changelog claimed neither, so it is unchanged.Pinned by.
WaitingTasksRollup5633Tests.PaddedAndTrimmedWaitType_AreOneRow_ProjectedTrimmed(live, pre-Wait names lose their trailing space, and two Hyperscale timer waits are ignored #4884 paddedLCK_M_Xplus trimmedLCK_M_Xon one session, and a lone paddedPAGEIOLATCH_SH) and the SQL pin now assertingGROUP BY ... rtrim(wait_type)andrtrim(wait_type) AS wait_type.wait_type, the class gaveTotal: 5, Failed: 2(the new live test and the SQL pin); restored afterwards.TraceFlagEmptyRunLivePostgresTestskeeps proving the reader, the viewer read and the analysis read return the same run times.Test plan.
Darling.TestsandLite.Tests:0 Warning(s),0 Error(s), before and after mergingorigin/dev.DarlingSessionReader,DarlingCollectorRunFactsReader,ViewerDataService.ConfigChanges,DailySummaryHorizon,DarlingRetentionHorizons,DarlingAnalysisService,FinOpsRecommendationFigures,DarlingFinOpsRecommendationsReader,EmptyTraceFlagRunsSql,WaitingTasksSql): 118 Darling.Tests files, 138 classes, plus every*Census*and*Ratchet*class, on a UTC rig with both preload libraries:Total: 2522, Failed: 0, Skipped: 0. Lite: 2 classes naming the changed Lite file (CollectorRunEmptyReasonTests,ConfigChangesReaderTests) plus Census, Ratchet andWaitingBlockedDatabaseFilterToolTests:Total: 113, Failed: 0.origin/devonce: Darling 646 tests (the touched classes,DarlingMcpSessionToolsTests,McpPageContractTests, Census, Ratchet),Failed: 0, Skipped: 0; Lite 95 tests,Failed: 0.Lite parity. Waiting tasks: Lite already groups and projects
rtrim(wait_type)and has the DuckDB tie and NULL fixture. Empty trace-flag runs: Lite's DuckDB SQL is a separate dialect and project and takes a different floor parameter, so it keeps its own copy. FinOps: Lite computes the same value inline asp95Cpu24h.What the seat should double-check.
EmptyTraceFlagRunsSqlisstatic readonly, notconstas the brief said: it embedsTraceFlagsBaselineFloorSql("$2"), a method call, so it cannot be a constant. The existingViewerDataService.EmptyTraceFlagRunsSqlandDarlingCollectorRunFactsReader.EmptyTraceFlagRunsSqlnames are gone; the one test that used the viewer name was updated. No other file referenced them.rtrimchange is visible only for rows stored before Wait names lose their trailing space, and two Hyperscale timer waits are ignored #4884.Changelog:
SECTION: Fixed
ENTRY:
REF:
[MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704]: MCP tool audit fixes: spills, empty reasons, FinOps, dropped databases, object stats (#5633) #5704
(The other three parts are internal: no changelog entry.)
F14: the FinOps index analysis and recommendations tools hand the host logger to the current-database lookup
Pushed to
fix/5633-mcp-tool-auditas f491226 (merged withorigin/dev, no conflicts).Why. F12 (e401d9b) gave the
CurrentDatabaseScopemethods an optionalILogger? loggerso their fail-open catch logs a Warning ("a SQL or ordinal mistake in it turns all of item 4 off"), and threaded the tool's logger fromget_top_procedures_by_cpuandget_query_store_clutter. Two Darling callers still passed none: the FinOps index analysis read (get_finopsviewindex_analysis) andget_finops_recommendations, so a failure of the dropped-database lookup there was silent.What changes.
GetFinOpsandGetFinOpsRecommendationstakeILogger? logger = nulljust beforeCancellationToken cancellationToken, the same position and form asget_wait_stats. NoDescriptionchanged (the logger is a DI seat with no description, so the tool list and the budget pins are untouched).GetFinOpspasses it toReadIndexAnalysisAsync, then toDarlingFinOpsIndexAnalysisReader.GetIndexAnalysisWithSnapshotTimesAsync,GetIndexAnalysisAsync,GetIndexCleanupInputsAsync, and the privateReadIndexObjectStatsAsync, which hands it toCurrentDatabaseScope.OmitDroppedAsync.GetFinOpsRecommendationspasses it toDarlingFinOpsRecommendationsReader.GetRecommendationsAsync, which gives it toCurrentDatabaseScope.ReadAsyncand to the compression check'sGetIndexCleanupInputsAsync. The tool'sPgSecondaryReplicaScope.ReadAsynccall, which hadlogger: null, now gets the same logger.ILogger? logger = nullbeforecancellationToken(CA1068 requires the token last). That moved the token for positional callers, so those now name it:ViewerDataService.FinOps.IndexAnalysis.cs(3 calls) and the live tests (DroppedDatabaseScopeLiveTests,FinOpsIndexAnalysisGoldenLiveTests,FinOpsIndexAnalysisLatestSnapshotLiveTests,FinOpsIndexAnalysisViewLiveTests,FinOpsRecommendationsSecondaryReplicaLiveTests,FinOpsRecommendationsToolParityLiveTests).DarlingWebEndpoints.BuildReadDispatch, which already holds the host logger) now passes it forget_finops,get_finops_recommendations, and also forget_query_store_clutterandget_top_procedures_by_cpu, whose web entries F12 left on the default null (the same gap, one line each).Caller list of
CurrentDatabaseScope(non-test), all pass a logger now.DarlingMcpDataTools.GetTopProceduresRanked(NarrowAsync): F12.DarlingQueryStoreClutterReaderread cost, plan churn, config (OmitDroppedAsyncx3): F12.DarlingFinOpsIndexAnalysisReader.ReadIndexObjectStatsAsync(OmitDroppedAsync): this lane.DarlingFinOpsRecommendationsReader.GetRecommendationsAsync(ReadAsync): this lane.CurrentDatabaseScope.NarrowAsync/OmitDroppedAsynccallingReadDroppedAsync/ReadSnapshotAsyncinside the class: pass their ownlogger.DroppedDatabaseScopeLiveTestsdirect calls.CurrentDatabaseScope, still no logger: the Viewer (ViewerDataService.FinOps.*) calls the readers with no logger. A WPF client has no hostILogger(noILoggeranywhere inPerformanceMonitor.Darling.Viewer), so it uses the optional default (null, silent), as before.Test plan.
CurrentDatabaseScopeLoggerThreadingTests: (1) reflection: both tools havelogger(anILogger, optional) right before theCancellationToken; (2) a source scan of every non-test.csunderDarling/that everyCurrentDatabaseScope.(ReadAsync|ReadSnapshotAsync|ReadDroppedAsync|NarrowAsync|OmitDroppedAsync)(call expression mentionslogger(floor: at least 5 calls found). Proved once on the old shape: removinglogger: loggerfromReadIndexObjectStatsAsyncmakes the scan fail (Total: 3, Failed: 1); restored, 3 of 3 pass.DarlingMcpFinOpsRecommendationsToolsTests.ToolSurface_ExactlyGetFinOpsRecommendations_WithOnlyServerNamepinned the exact parameter listpostgres, server_name, cancellationToken; it now pinspostgres, server_name, logger, cancellationToken, and still asserts onlyserver_namecarries aDescription(the client-visible surface is unchanged).Darling.Testswith--no-incremental:0 Warning(s),0 Error(s).*CurrentDatabaseScope* *FinOps* *DroppedDatabaseScope* *DarlingMcp* *DarlingWebEndpoints* *WebDatabaseFilter* *WebReadCancellation* *WebReadReach* *WebExceptionText* *Census* *Ratchet* *McpToolsListBudget* *McpServiceParameter* *InventoryPin* *QueryStoreClutter* *TopRanking* *McpLatestSnapshotStamp* *EngineCapabilityMiss* *McpReadCommandTimeout* *WebTopProcedures* *ReadLatencyWeb* *Viewer* *DocCommentHygiene* *TsqlConventionGuard* *LiteReadsTrait*, after mergingorigin/dev:Total: 7233, Failed: 0, Skipped: 789. The skips are the live classes (noDARLING_TEST_PG).DroppedDatabaseScopeLiveTests, the FinOps index and recommendations live classes): compile with the new signatures but were not run, since no rig was started for a signature-only change. CI runs them.Lite parity. Nothing to mirror: Lite logs through
AppLogger.Warnalready (brief). No Lite file changed.What the seat should double-check.
loggerbeforecancellationToken) is why six live tests and three Viewer lines changed to a named token; that is mechanical.get_query_store_clutter,get_top_procedures_by_cpu) done here rather than in F12's section.Changelog: none (internal logging).
F15: merge origin/dev (V179 Client Timeouts, #5640/#5714)
Merge commit
888911038e928c217dbac1c447514f95c6dfd0de, pushed tofix/5633-mcp-tool-audit. No code changed beyond the two conflict files.Conflicts (2), both kept both sides
Darling/README.md, worker-sizing paragraph: kept the branch's RAM-bounded paragraph (8 GB example, floor and ceiling, 142 measured jobs) and added dev's parenthetical to it: "72 hypertables (the 71 collector tables pluscollection_log; the 72nd collector table,attention_events, is a plain table with its own 30-day delete and is not counted)". Dev's "Today that is 74 and 85" lead-in was dropped, because the branch states the same 74 and 85 as the 4 GB floor. Dev's 71 -> 72 collector count (43 SQL Server) merged cleanly.Darling/Darling.Tests/ReadmeDerivedCountPinTests.cs, worker-sizing pin: the branch's regex ("For an 8 GB host that is ...", figures fromDarlingManagedPostgres.DeriveWorkerSettings) with dev's 4th figure,TimescaleSupport.HypertableTables.Count, and dev's comment.Recounted figures (the pin produced them; nothing hand-typed): worker sizing did not change. 8 GB host is 128 background workers and 139 worker processes for 72 hypertables (
HypertableCount, 71 collector hypertables +collection_log) and 29 aggregates. The attention table is plain, so the hypertable count is unchanged. Collector total 72 (43 SQL Server, 29 PostgreSQL).Test plan (rig on port 5684, UTC, both preload libraries, stopped afterwards)
ReadmeDerivedCountPin5/5,DarlingManagedPostgresTests231 (3 skipped),JobCountWithinWorkerSizingTests2/2 (live),CiClusterWorkerSizingTests5/5,CiComposeWorkerSizingTests4/4,*Ratchet*15/15,*Census*548/548.*Census*67/67,*Ratchet*matches 0 tests, CrossAppMcpToolInventoryPin 7, CrossAppGuardCiGate 20, CiSelectMapSelectGuard 31, CiSelectReplayGuard 4: all passed.Seat should double-check: the other README-reading test classes not in the list above were not run; CI covers them.
Changelog: none.
Answers to the bot review of 168d822
Claude posting for Erik Darling. Each finding from review 5496066203 and its five inline comments, with where it was answered.
Refuted claims
first(blocking_session_id ORDER BY wait_duration_ms DESC NULLS LAST, blocking_session_id ASC NULLS LAST), filtered to other-session blockers (989495f). That gives PostgreSQL's row on a tie and keeps a blocker whose wait time is NULL. Pinned byWaitingTasks_TiedAndNullWaitThreads_PickTheSameRowAsDarling, the three-row case the review asked for.Inline findings
get_top_procedures_by_cpuclaims "recorded nothing" after it narrowed the filter: fixed in F12, both products. Both page shapes now carrydatabases_left_outand a note, and the empty sentence covers only the databases the server still has. F12 also fixed the same problem in Query Store clutter and FinOps index analysis. FinOps recommendations has no "nothing found" sentence, so it needed no change.wait_type: fixed in F13. Darling now groups on and returnsrtrim(wait_type), as Lite does, pinned by a live test with a pre-Wait names lose their trailing space, and two Hyperscale timer waits are ignored #4884 padded row.DarlingRetentionHorizons.EmptyTraceFlagRunsSql), used by the MCP reader and the viewer. The analysis copy readscollection_log, notv_collection_log, and its doc says why. Lite's copy is DuckDB SQL in another project.p95Cpu7dnow fed a 24-hour P95: renamedp95Cpu24hin F13.Riskiest lines
CurrentDatabaseScope's catch-all: it still fails open, by design, but it now logs a Warning each time. Lite logs throughAppLogger.Warn. Every Darling MCP tool and web read entry that reaches it passes its logger (F12, F14). The web viewer's readers have no logger, as before, and keep the silent default. The live classes that pin the read run in CI's Darling PostgreSQL shards.array_aggrollup: pinned byWaitingTasksRollup5633Tests, live in CI. F13 added the padded wait-type case.compare_analysisdefault baseline: intended (item 12). Unit-pinned byMcpToolAudit5633L3Tests.ValidateBaselineWindow_*.Trap: a latest-snapshot read must say when: this does not apply as written. Here the snapshot only decides which databases to leave out. The rows are windowed procedure stats with their own times. The payload now names what it left out and why.
Reviewer checklist
get_waiting_taskson pre-Wait names lose their trailing space, and two Hyperscale timer waits are ignored #4884 rows: F13's live test.get_top_procedures_by_cpuover 168 h with only dropped-database activity: F12's live test (Darling) and tool test (Lite), each shown failing on the old shape.Not re-measured: L6's plan claim (one indexed read per call, look-back capped at 7 days) was never timed on a large
database_states.W2A: the MCP role can write the collection caveats it persists (item 13) - replaced by F8
Lane W2A first fixed item 13 by granting the
mcprole INSERT, UPDATE and DELETE oncollect.analysis_collection_caveats. The wave 2 security review found that this let anas_ofrun ofanalyze_serveroverwrite the current caveats, and let any MCP client rewrite caveat rows for any server. F8 removed the grant and took a narrower approach: only the scheduled analysis pass writes that table, so themcprole needs no new privilege. See the F8 section. This section has no changelog entry; F8's entry replaces it.W2B: enough background workers for the store's jobs (item 14) - sizing replaced by F9
Lane W2B measured the problem on a rig. A store built by the migrations carries 142 TimescaleDB jobs (94 compression, 23 refresh, 23 retention, 2 built in). With the old 74 workers, a restart with every job overdue logged 61 "out of background worker slots" launch failures. They were retried and every job ran, so the cost was a noisy, late first pass. W2B raised the workers to a fixed 160. The wave 2 review asked for the memory cost of that on a small host, so F9 measured the restart burst and replaced the fixed number with sizing by host memory. See the F9 section. This section has no changelog entry; F9's entry replaces it.