Release 3.5.0 - #2381
Merged
Merged
Conversation
…termark (#2312) (#2333) * query_store plan/text fetch goes activity-driven: the store is the watermark The invariant 40-110s-per-run bill (#2312) had a named mechanism at last: the #2210 watermark walk's daily expiry was supposed to be replaced by a re-verify cursor that was built, tested, documented, and never wired (Finding 4) - so catalogs whose full walk needs more than a day expired MID-walk, restarted from plan_id 0, and looped the full catalog fetch forever. TouchSql, the liveness refresh the dimension GC depends on, had the same story: designed as 'the whole of the protection', zero callers (Finding 3), latent only because the perpetual walk's re-upserts were accidentally standing in for it. The reshape retires all of it in one shape change: the cycle's collected rows name their plans/texts; one touch-and-probe round trip per database refreshes map/dim liveness AND returns the missing set plus per-cycle in-place-rewrite / Query-Store-reset detection via the live hashes the payload already carries; the fetch selects exactly those ids under the same byte-budget arithmetic. A caught-up database issues NO target query. A reset recovers as the normal path. A dormant plan resuming execution is fetched the cycle it resumes. V77: query_store_plan_map.digest goes nullable (the NULL-XML content-less marker, so unpersistable plans read as known instead of riding every fetch list), query_store_text gains query_hash (the reset detector), and the orphaned planwm:/textwm: state rows are deleted wholesale. Budget- deferred ids carry over in memory so a plan referenced once cannot starve; target-side-gone ids drop only on a provably-uncut pass. Upserts COALESCE digest and hash toward knowledge, never absence. Retired: QueryStorePlanXmlState's watermark half (sizing stays - it caps server-side decompression), QueryStoreTextState wholesale, the cursor members, and the planwm:/textwm: entries in the shared prune set. Tests: QueryStorePlanWatermarkTests is reborn as QueryStorePlanFetchTests (new builder/probe/upsert shape pins + the surviving sizing tables, harness- executed against the built assembly), the V77 rung ceremony lands in ActivityDrivenPlanFetchStoreTests with the V74/V75/V76 files demoted, and QueryStoreFetchProbeLivePostgresTests round-trips the probe verdicts, liveness stamping, hash adoption, and the NULL-digest marker against a real store. Closes #2312 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * CI catches: two doc-stacks from splice insertions, the seed-cap arithmetic, and two source pins Both splice edits landed between a member's doc block and its body - the fetch method's doc stranded above the new extractor helpers, and V76's rung doc stranded above the V77 insertion. Third instance of this exact trap; both docs now sit with their members and a local scan mimicking the hygiene test confirms zero stacks. The seed candidate cap pin claimed the issue text's rounded ~118 where ceil(12MB/160KB*1.5) is 116. The text call-site literal QueryStoreTextStoreTests pins is restored to one line, and the runner's comments stop naming the retired prefixes literally so the retirement source pin can hold. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…g_proc (#2340) (#2341) Measured against a live Aurora PostgreSQL 17.7 cluster (looker-poc, RDS engine aurora-postgresql) as the least-privilege darling_monitor role: SELECT count(*) FROM pg_proc WHERE proname = 'aurora_version' -> 0 SELECT aurora_version() -> 17.7.2 So the old probe read a genuine Aurora target as stock PostgreSQL. Both PgWaitStatsCollector and PgStatementStatsCollector gate on IsAurora, so one wrong boolean silently dropped the two most valuable PostgreSQL reads -- pg_stat_statements was installed and available on the cluster, so that collector was skipped purely by the flag. --test-connection reported '6 of 8 ... (skipped: pg_statement_stats, pg_wait_stats)', which reads as ordinary informational output rather than a capability failure. The marker moves out of the detection query into its own statement that CALLS the function: a stock-PostgreSQL 42883 undefined_function is caught and read as 'not Aurora' (the expected negative, and the wrapping the old comment claimed but a catalog subquery never needed), any other failure is also caught in that direction so an optional-collector probe can never fail a connect, and both are logged at debug so 'why is this Aurora cluster reading as stock' is answerable from the service log instead of requiring a live psql session -- which is what diagnosing this actually took. The test pins the absence too: re-adding a pg_proc fallback would restore a check that is wrong precisely where it matters. Closes #2340 Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…roughput (#2266 item 1) (#2345) * Assert what the scale test can prove, not TimescaleDB's throughput (#2266 item 1) ScaleTest_JobDurationGrowsWithVolume_... required d10 > d1 between two sub-second compression-job durations and kept failing on diffs that cannot reach it (d1=970/d10=863, then d1=689/d10=689). Measured it rather than reasoning about it. Fifteen consecutive runs of the exact sequence against TimescaleDB 2.29/PG17: the chunks compress perfectly every time (counts 1, 2, 3; per-day rows exactly 2000/50000/500000), which refutes the standing suspicion that both runs were compressing nothing. But a 10x volume increase buys only ~3.2x the duration - about 85ms of absolute signal, because compression cost is largely fixed per run. CI's baseline for the same pair is 690-970ms, twenty times that fixed cost, so the volume-dependent part there is ~10% of the measurement's own magnitude and sits inside the variance of launching a background worker on Windows. No threshold, ratio or volume rescues that: at ~0.19ms per thousand rows it would take millions of rows per chunk to clear a variance nobody has measured on the platform that fails. The byte-identical pair was also never as improbable as it looked, because that pair is only read when the test FAILS, which selects for differences already near zero. Replaced with something strictly stronger, not looser: each measured run must have compressed the chunk its own seed created, and that chunk must hold exactly the seeded row count. Exact counts instead of two timings. A negative control proves the difference rather than assuming it. Seed the 10x rows into a not-yet-eligible chunk and old and new both fail (new one names compressed=2). Seed them into the 1x chunk and the OLD assertion passes 3/3 with a 6-8x ratio while the fixture has quietly stopped producing two chunks at two volumes - only the new assertions catch it (rows=[2000,550000], total=2). The shipped assertion was not merely flaky, it was blind to the fixture defect it existed to guard. The product's own claims are unchanged and still asserted: a real duration is measured, the V56 series records both readings in order, and the real evaluator fires the cadence alert from a real reading. One gap closed on the way past - d10 > 0 was never asserted, and ReadJobDurationMsAsync maps a NULL duration to 0, so a 10x run with an unmeasurable duration satisfied the telemetry check as 0 == 0 and passed. Renamed so the test stops claiming what it no longer measures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Spend the describe query only on a failure (review catch) Assert.True's message argument is a plain string, so the interpolated DescribeJobWorkAsync call was evaluated eagerly - two live catalog queries per run to build a message nobody reads on the passing path. That also contradicts the helper's own contract, which says it exists to explain a failure. Branch and Assert.Fail instead, so the diagnostic runs only when there is something to diagnose. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…regenerated and the Extensions family aligned (#2336) * Bump the nuget-patch-and-minor group with 7 updates Bumps Microsoft.Extensions.Hosting from 10.0.10 to 10.0.11 Bumps Microsoft.Extensions.Hosting.WindowsServices from 10.0.10 to 10.0.11 Bumps Microsoft.Extensions.Logging.Abstractions from 10.0.10 to 10.0.11 Bumps Microsoft.NET.Test.Sdk from 18.8.1 to 18.9.0 Bumps ModelContextProtocol from 2.1.0 to 2.2.0 Bumps ModelContextProtocol.AspNetCore from 2.1.0 to 2.2.0 Bumps System.Security.Cryptography.ProtectedData from 10.0.10 to 10.0.11 --- updated-dependencies: - dependency-name: Microsoft.Extensions.Hosting dependency-version: 10.0.11 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: nuget-patch-and-minor - dependency-name: Microsoft.Extensions.Hosting.WindowsServices dependency-version: 10.0.11 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: nuget-patch-and-minor - dependency-name: Microsoft.Extensions.Logging.Abstractions dependency-version: 10.0.11 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: nuget-patch-and-minor - dependency-name: Microsoft.NET.Test.Sdk dependency-version: 18.9.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: nuget-patch-and-minor - dependency-name: ModelContextProtocol dependency-version: 2.2.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: nuget-patch-and-minor - dependency-name: ModelContextProtocol.AspNetCore dependency-version: 2.2.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: nuget-patch-and-minor - dependency-name: ModelContextProtocol.AspNetCore dependency-version: 2.2.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: nuget-patch-and-minor - dependency-name: System.Security.Cryptography.ProtectedData dependency-version: 10.0.11 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: nuget-patch-and-minor ... Signed-off-by: dependabot[bot] <support@github.com> * Regenerate the lock files and align the Microsoft.Extensions family the group split Two separate failures behind one red check: NU1004 — the bump edited Directory.Packages.props but left nine packages.lock.json files describing the old CentralTransitive versions, so CI's locked-mode restore refused. Regenerated with --force-evaluate. NU1605 — the group bumped Microsoft.Extensions.Hosting to 10.0.11 while leaving Configuration, Configuration.Json and Logging pinned at 10.0.10. Hosting 10.0.11 requires >= 10.0.11 of all three, so central pinning resolved them DOWN and the downgrade is an error here. Those four move as a family; aligning them at 10.0.11 is the fix, not pinning Hosting back, because the already-present Hosting.WindowsServices and Logging.Abstractions were at 10.0.11 too. Verified locally: --force-evaluate then --locked-mode both clean (locked mode is what CI runs, and it is the one that was failing), and all seven projects build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…rver" (#2339) (#2342) * Darling MCP declares its peer stores instead of answering "unknown server" (#2339) Tier 1 of the multi-store fix: disclosure only, no federation. With the fleet split across several boxes -- one store each for the SQL Server primaries, their readable replicas, and PostgreSQL -- every box's MCP server answered over ITS store alone, so a server monitored by a sibling resolved as not-found. That is indistinguishable from a server nobody monitors, and with a deliberately split fleet it is now the normal case rather than an edge. An optional "peers" block in darling.json (thisStoreCovers, plus per-peer name / covers / optional matches name-substrings) is disclosed at the three places an agent forms its picture of the fleet: - the MCP instructions gain a Fleet Coverage section, placed between the read-only preamble and the tool census so an agent learns WHICH store it is talking to before it starts planning against the tool list; - list_servers gains this_store_covers, peer_fleets, and a peer_note; - the server-resolution miss appends "not monitored HERE -- matches the declared coverage of <peer>" to the existing available-servers listing. There is no credential, no address and no connectivity behind any of it: the service never contacts a peer, cannot read one's data, and cannot tell whether one is running -- and every message says so, because an agent told a sibling exists will otherwise try to route a query at it. Since all of this text is sent verbatim to every connected MCP client, PeersConfig.Validate refuses peer text that looks like a connection string or credential; failing open there would broadcast the secret. Two deliberate shapes. "matches" is plain case-insensitive substrings, no globbing and no regex -- it exists to answer "which region/role prefix is this name?", and a pattern language would be a config surface with its own failure modes; blank entries are dropped, because an empty substring matches every name and would make one peer claim the whole fleet. And an empty peer_fleets carries its own note: it means EITHER this is the only store OR nobody declared the siblings, this server cannot tell those apart, so it says that rather than letting an empty array read as "you are looking at the whole fleet." The snapshot is ambient (immutable value behind a volatile field, published from the worker and the MCP host) rather than injected: ~90 tool methods resolve a server name through DarlingServerResolver taking only an NpgsqlDataSource, and threading a parameter through all of them plus the web read dispatch would touch every one to deliver a constant. Snapshot.Empty is the default, so with nothing declared all four surfaces are byte-for-byte what they were -- the resolver miss included, pinned by an exact-equality test. Lite has no peers concept and no central store, so it gets no twin. Verified: Darling.Service, Darling.Tests and Lite.Tests all build clean (EnableWindowsTargeting). The Windows-only suites cannot run on macOS, so a throwaway net10.0 harness ran 70 assertions against the real build -- config parse/validate, normalization, matching, the instructions section and its placement, the rendered list_servers JSON, all four resolution-miss shapes, and the ambient publish -- plus the network-config editor's text surgery against the edited darling.sample.json (its real fixture, whose last root member moved). Negative control: neutering the disclosure turns 9 of those red. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * The empty-registry answer discloses the peers too (#2339) Self-review gap: list_servers answers an empty registry with prose rather than the JSON envelope, so it early-returned past the peer block -- the one path where the declaration silently vanished. That is the worst place to lose it. An empty registry means a fresh or just-restarted box, so "No servers are registered yet" with no mention of the siblings is the strongest possible version of the wrong conclusion this feature exists to prevent. The prose answer now carries the peer list and this store's coverage explicitly, and is unchanged when nothing is declared like every other surface. Also cross-references the peers section from the MCP tool inventory in Darling/README.md. Verified: all three projects build clean; the harness gained four assertions for the new path (and prints the message verbatim), 74 passing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Move the credential guard into the publish, where the broadcast actually is (#2339) Two real review findings, both reproduced before fixing. 1. The guard never covered the path that broadcasts. PeersConfig.Validate ran only from DarlingConfig.Validate, which only the worker calls -- and the worker's abort is a `return` from its own hosted service, not a process exit (no StopApplication, no Environment.Exit anywhere in Program.cs). DarlingMcpHostService loads its own copy of darling.json and deliberately never validates it, because its fail-closed checks are host-local by design. So a credential pasted into a peer description stopped collection and then went out to every connected MCP client through the instructions, list_servers, and every resolution miss -- with the worker having aborted before its own publish, the host's unvalidated publish became the ONLY value in the ambient snapshot. Fixed at the category, not the call site: DarlingPeerDirectory.Publish now validates and refuses, returning a PublishResult that carries the problems so a caller cannot take the snapshot and drop the reason. The ambient snapshot can only be written through Publish, so a future third publish site cannot reintroduce the hole. A refusal publishes Snapshot.Empty -- the whole block, not the valid subset: a block that failed validation is one the operator has not finished, and half a disclosure would state coverage that may be wrong while the log says the config is broken. Losing disclosure for one restart is recoverable; leaking a credential is not. Both call sites log the problems at Critical. 2. An explicit "stores": null skipped the thisStoreCovers guard. System.Text.Json assigns null over the property initializer (an omitted key leaves the default), and the self-text check sat after the per-peer loop behind an early `peers?.Stores is null` return -- so a credential in thisStoreCovers validated clean and was disclosed on all three surfaces. The fix is ordering, not another check: an unconditional guard cannot be bypassed by a shape nobody enumerated. Also: the multi-claimant resolution miss now agrees in number. Two peers can legitimately both claim a name through overlapping `matches`, and the follow-on sentence said "That is a SEPARATE Darling store" about a list of two. Verified: all three projects build clean. Three new xUnit facts cover the two findings and the pluralization. The harness reproduced both defects first (the "stores": null config validated with zero problems and the credential reached the rendered disclosure), then grew 17 assertions for the fixes -- including that the leaking text appears in neither the instructions nor a resolution miss, and that the VALID peer in a refused block is not disclosed either. 114 assertions passing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Say the list_servers shape change out loud instead of claiming "unchanged" (#2339) Review finding, and a fair one: "with nothing declared all four surfaces are byte-for-byte unchanged" overstated it. Three are -- the instructions (same reference), the resolution miss (pinned by exact equality), and list_servers' empty-registry sentence. list_servers' JSON envelope is not: it carries this_store_covers / peer_fleets / peer_note on every response, declared or not, so a client comparing that tool's exact shape sees three new keys on upgrade even if it never writes a peers block. That stays as it is -- it is the design, not an oversight. An empty peer_fleets means EITHER "this is the only store" OR "nobody declared the siblings", and a note that only appeared once peers were declared would say nothing in precisely the case that produces the wrong conclusion. But an overstated claim is worse than an admitted exception, so the exception is now named in the CHANGELOG, in the test class doc, and in Darling/README.md, next to a note that a failed peers block is refused whole. Documentation only; no behavior change, and the pin that caught the discrepancy (ListServersEmptyPeerFleets_SaysWhatItDoesNotProve) is left exactly as it was -- it was asserting the right thing all along. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…#2343) * Diagnose an unreadable install location in the service itself (#2185) The reported failure had three halves and two were already shipped: #2186 decoded the loader status behind `initdb failed (exit code -1073741515)`, help, and #2187 taught install-darling.ps1 to refuse a user-profile or network install root. The remaining gap is that the installer only guards installs that go through it. The README's manual `sc create` path, and anyone who registers the exe by hand, bypass it entirely and still land in the reporter's experience: an empty `Output:`, a bare exit code, then a missing pg-admin-credential.dpapi and advice to start the service once, which they had. Nothing named the install directory. DarlingInstallLocation classifies the install directory and is called as the FIRST thing DarlingWorker.ExecuteAsync does - ahead of reading darling.json, which an unreadable tree also takes out, and long before the managed-Postgres bootstrap - so the cause sits ABOVE the failure it predicts in the log an operator reads bottom-up. One critical line, naming the offending path, the account the service is actually running as, why that account cannot read that location, and where to move it. Diagnose and continue rather than refuse to start, deliberately: the same asymmetry the installer applies to an upgrade, for the same reason (#2187's rejected option 2 - an operator may have granted the tree read + execute by hand, and stranding a deployment that runs today would be worse than the disease). Silent on a console run, because an interactive run IS the profile owner and test-driving the exe from a Desktop folder is something the README suggests; unreachable on non-Windows hosts, so the compose deployment cannot trip it. The decision table is deliberately the installer's own rather than a second definition of "a location that cannot work": profile root read from Windows instead of a hardcoded C:\Users, plus %USERPROFILE%; UNC excluding the \\?\ long-path prefix; a drive letter whose type is network. Making the C# the single source and having the script call it is the better end state and is NOT done here - it would change the installer's behavior, which is not this change's to make - so drift is caught instead: the new parity test runs BOTH implementations over ONE shared table under Windows PowerShell 5.1, lifting the script's own composition lines verbatim rather than retyping them, and fails if the two ever disagree. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Read the profile root from ProfilesDirectory, not %PUBLIC%'s parent (#2185) Review catch, and a real false negative. MachineProfileRoot derived the machine's profile root from %PUBLIC%'s parent, on the stated belief that Windows keeps PUBLIC in step with ProfilesDirectory. It does not: ProfilesDirectory and Public are two INDEPENDENT REG_EXPAND_SZ values under HKLM\...\ProfileList that merely default to the same tree. An administrator who relocates profiles to D:\Profiles - a documented, supported move - without also moving Public leaves %PUBLIC% at C:\Users\Public, so the service would have answered C:\Users while install-darling.ps1 answered D:\Profiles, and an install under the box's real profile root would have passed silently. That is a false negative on exactly the case this check exists to catch, on exactly the box where a missed check costs the most - and the whole point of sharing the installer's decision table was not to have a second definition that can diverge from it. Reading ProfileList\ProfilesDirectory directly removes the divergence instead of documenting it, and needs no package reference: the Registry types are in the shared framework's ref pack for net10.0, annotated Windows-only, which the class already is. Same fallback as the installer (%SystemDrive%\Users); an unreadable ProfileList still does not skip the check. The blind spot the reviewer also named is closed executably. Both existing parity tests INJECT the profile root - that is what makes the table comparable, and it means neither side's own lookup was ever exercised, which is why nothing caught this. TheInstallerAndTheService_ReadTheProfileRoot_ FromTheSamePlace supplies nothing: it runs the installer's shipped Get-ProfilesDirectory and requires MachineProfileRoot to answer the same on whatever box the tests run on. It passes trivially on an unrelocated box, which is fine - its job is to fail the moment the two stop reading the same thing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * The fallback profile root is rooted, not drive-relative (#2185) Second review catch, and a worse one than it looks. The registry-unreadable fallback built the root with Path.Combine(systemDrive, "Users"). %SystemDrive% is documented to be a bare drive and colon with no trailing separator, and Path.Combine treats a trailing VOLUME separator exactly as it treats a trailing directory separator: it inserts nothing. So the fallback produced "C:Users" - a drive-relative path, which Path.GetFullPath then resolves against the process's current directory on that volume rather than against the volume root. For a service that is not C:\, so the profile check would have silently stopped matching real profile installs on every box whose ProfileList cannot be read: the one box the fallback exists for. It was also a silent divergence from the installer, whose Join-Path 'C:' 'Users' does the right thing. The composition is now its own pure function, ProfileRootForSystemDrive, for one reason: the registry read succeeds on every box CI runs on, so the fallback branch was unreachable from a test, which is exactly why this shipped. It is pinned by a table (bare C:, C:\ with a separator already, D:, unset, blank, null) asserting drive + separator + Users and specifically NOT bare concatenation, plus an assertion that whichever branch the shipped lookup takes, MachineProfileRoot returns a fully-qualified path. Note for the record: the macOS harness could not have caught the original defect and does not claim to - Path.Combine's volume-separator special case is Windows-only behavior, and Path.Combine("C:", "Users") already yields "C:/Users" there. The Windows test job is what pins it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Name both store modes in the diagnosis, not just the managed one (#2185) Third review catch, and a fair one. The message narrated the managed bootstrap's failure chain - initdb.exe at 0xC0000135, then a missing pg-admin-credential.dpapi - unconditionally, but that chain only exists when postgres.managed is true. This message is deliberately built BEFORE darling.json is read, because an unreadable tree takes the config out too, so it cannot know which mode is configured. An operator pointed at their own PostgreSQL has no initdb to fail, and sending them to look for one would spend exactly the credibility the message exists to have. It now names both, in two sentences: the managed default's chain as the default's, and for bring-your-own the failure they will actually see, which is "Cannot load configuration" from a darling.json sitting in the same unreadable folder. That second one was missing entirely even though the call site's own comment cites it as a reason to run this check early. The ordering is unchanged and still pinned - naming both costs two sentences, where deferring the message until after config load would cost the ordering that makes it useful. Describe_NamesThePathTheReasonAndTheRemedy now requires both modes and the config-load string, for every verdict. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Record the extended-length path residual, both halves (#2185, #2348) Review catch, documented rather than fixed, deliberately. \\?\UNC\server\share is the extended-length spelling of a REAL share and the \\?\ exclusion in the UNC test is wholesale, so it is waved through with no diagnosis - the same shape as the already-recorded \\?\C:\Users\bob gap. install-darling.ps1 has both identically, which is why neither is fixed here: a carve-out on one side alone is exactly the drift the shared decision table exists to prevent, and changing the installer's behavior is not this change's to make. Filed as #2348 with the fix and the guardrail; both shapes are now table rows, so the cross-language parity test goes red the moment one side moves without the other. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…ally use (#2344) (#2346) With #2333's catalog walk gone, the per-database split named the phase that does not subside after catch-up: wm, 1-3s per database per cycle. It is a read against OUR store, not the monitored server -- an unbounded MAX over a non-partitioning timestamp, so it touches every chunk that database has. Measured on the live use1 store (106 GB, 5 chunks), same server/database pair: unbounded, cold: 25,766 buffer reads + 195 written (temp spill) unbounded, warm: 228 ms bounded to 3h: 29 ms, 5 chunks excluded The unbounded cost is a function of store size and cache residency rather than of the monitored workload, so it degrades precisely where an operator is weakest: a long-lived store, a busier Query Store, slower disks. The bound changes no answer, and that is the whole justification: every consumer ends at max(stored, now - MaxCatchup), because ClampCatchup floors anything older and a null result falls back to query_store's 60-minute first-run window -- the same instant as the floor. So a row below the horizon cannot move the result whether it is found or not, and the unbounded MAX was paying to confirm a value the clamp would have produced anyway. The predicate goes on collection_time (the partitioning column) because a predicate on the watermark column alone prunes nothing, and a row's watermark can never exceed its own collection_time, so nothing qualifying hides behind it. Bounded for query_store ONLY, on both hosts, name-guarded like the other query_store-specific behavior in these runners: a ring-buffer collector whose legitimate catch-up spans days must keep reading its whole history, and the floor would silently truncate it. WatermarkPolicy.ReadFloor carries the reasoning and the measurements; its tests pin that the floor sits strictly older than the clamp horizon and that a buried watermark and a not-found one reach the same instant. Closes #2344 Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…ion (#2348) (#2354) * Strip the extended-length prefix before classifying the install location \\?\UNC\server\share is the long spelling of a REAL share, and \\?\C:\Users\bob of a REAL profile, but both guards excluded \\?\ wholesale and waved them through with no diagnosis (#2348). Skipping a check is not the same as passing it. The exclusion was right about one case and wrong about the rest: \\?\C:\PerformanceMonitorDarling is an ordinary local root written the long way and must not be refused. So the fix is a normalization rather than a deletion — strip the prefix at the single entry point, then let every existing rule run against the ordinary spelling. One place knows the prefix exists; everything downstream is written against real paths, and the carve-out is gone. The UNC form is matched first because it is the longer prefix: stripping \\?\ first would leave the nonsense UNC\server\share, which is neither a share nor a local path. It is matched case-insensitively because Windows accepts \\?\unc\ too, and a miss there would re-open the hole for whoever typed it that way. install-darling.ps1 gets the same treatment via Convert-ExtendedLengthPath, applied to a separate $classifyRoot so the installer still writes to $root exactly as given — the prefix instructs the path parser and is not part of where the install lives. The three table rows that pinned this as a known residual now assert the fix, joined by lowercase, share-root and mapped-drive cases. The parity test lifts the normalization line out of the script alongside the two rule lines, so the pair still cannot drift. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Point the profile-check pin at $classifyRoot LocationGuard_ReadsTheProfileRootFromWindows_RatherThanAssumingCUsers pins the shipped composition line by substring, and #2348 renamed the variable it classifies. The rule it guards is unchanged -- the operator's own profile is still checked alongside the machine profile root -- but it is now checked against the normalized spelling, which is the whole point of the change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* Stop pretty-printing MCP tool results The only consumer of an MCP tool result is a language model, and indentation buys a model nothing (#2350). It was one property on one shared object in Common, so both SKUs move together: 78 call sites across the Darling and Lite MCP surfaces already route through McpHelpers.JsonOptions. The two readers that keep their own options for the /api/* twins get the same treatment, so the web endpoint and the tool still serialize an identical shape. The saving is payload-shaped and should not be oversold: 23% of the BYTES on a 15-field record array, 36% on a narrow one, and the TOKEN saving is smaller than either because BPE tokenizers pack runs of spaces efficiently. It costs nothing, which is the argument. Scope was the whole risk here, not mechanism. The config files people open and hand-edit keep indenting -- ServerManager, ProfileManager and ScheduleManager carry their own options and are untouched -- because flipping this on a file writer would turn servers.json into one unreadable line, which nobody notices until an awkward moment. The new test pins both directions: the tool output carries no layout whitespace, and those three writers still say WriteIndented = true. Verified nothing depended on the layout first: no test asserts on serialized MCP output containing newlines, the 24 files that parse tool JSON use a parser, and the tests that touch this options object assert field NAMES rather than shape. Found while reviewing #2286, which benchmarked a third-party wire format against our indented output -- so some of the win it reported was a serializer flag. Credit to @blackwell-systems for the measurement that surfaced it; this half carries no dependency. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Assert JSON content without asserting JSON layout CI caught what my pre-change check missed. I grepped for tests asserting on serialized output containing NEWLINES and concluded nothing depended on the layout. Eighteen assertions depended on the space after the colon instead -- "severity": "Critical" only exists under WriteIndented -- and four test classes failed without a single thing they were testing having changed. Those assertions read as claims about content (this field serialized with this value, an enum as its string name rather than its ordinal, a null that stayed null) but were written as claims about formatting. So the fix is to make them mean what they looked like they meant, not to retype the literals in compact form -- which would leave the same trap armed for whoever changes the formatting next. JsonAssert.Contains/DoesNotContain normalize both sides by dropping whitespace BETWEEN tokens while preserving whitespace INSIDE strings, so "a": "b c" and "a":"b c" compare equal and the two-space value in "b c" survives. Escaping is tracked so a \" inside a string does not end it and a \\ before a quote does not escape it; get that wrong and the scan falls out of the string, starts stripping real spaces from values, and the assertion silently compares something else. Deliberately still substring assertions rather than a full parse: they check that one field serialized a particular way without pinning the shape of the envelope around it. Four format-coupled assertions elsewhere are deliberately untouched -- exported darling.json, the network config editor, a stored command result and a static options constant. Those are config files people read and a literal, none of them MCP output, and CI passed them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…e did (#2353) (#2356) * Serve get_query_trend from the tier that can answer, and say which one did The read went to raw query_stats only, and the raw tier of a ROLLED table is dropped at 4 days independently of the collector's much longer advertised retention (#2353). So a request for 168 hours returned whatever had not aged out, under a label still saying 168. The empty path was worse than the short array. It asserted "No history found ... within the last 168 hours" over a span the read never covered -- for a query whose history had simply aged out that is a false statement, not an incomplete one, and an agent acts on it by concluding the query never ran. It now says which tier it searched and tells the caller to confirm the hash before drawing that conclusion. Tiering is by the age of the window's OLDEST point measured from WALL CLOCK, matching ComposeSourceRouter. Writing the test caught this being wrong first time: comparing the start against the window's END makes a two-hour window from ten days ago look "recent" -- it is recent relative to its own end -- and routes it to a tier that dropped those rows six days earlier. Retention answers to now, so now is the parameter. A window past the horizon is served ENTIRELY from the hourly rollup rather than stitched: a series whose bucket width changes partway is a worse answer than a coarser consistent one. The rollup keeps executions, CPU and elapsed and nothing else, so the other eight columns are selected as typed NULLs and surface as null -- never zero, because on an aggregate row a zero reads as "none observed", a measurement nobody made. The response now carries source, effective_hours_back, bucket, truncated and a note naming exactly which fields are unavailable and why. Both projections are pinned to identical ordinals, since one mapper serves both and a column added to one alone would mis-map silently rather than fail. Reported by @carlei1978, who traced it to the SQL rather than stopping at "the numbers look short". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Restore GetQueryHistoryAsync's doc block, and stop tiering on the window's end DocCommentHygieneTests caught the splice: extracting ShouldUseRawTier put its doc block below GetQueryHistoryAsync's, so the method's summary ended up stacked on RawTierMargin's and the method itself was left undocumented. Moved rather than deleted -- the test's own message warns that seven of the eight found in #1745 were displaced blocks whose real member had been left bare, and deleting them would have lost documentation rather than deduplicating it. This was the eighth. Fixing that surfaced a real bug underneath it. GetQueryHistoryAsync was calling ShouldUseRawTier(startUtc, endUtc) while the parameter means nowUtc -- exactly the mistake the test ANarrowWindowInThePast_StillTakesTheAggregate exists to catch, sitting in the one call site the test does not cover. It happens to work today because the only caller passes now as endUtc, which is the worst kind of correct: right by coincidence, and silently wrong for the first caller that asks for a historical window. Tiering now measures against the real clock, with an optional nowUtc for tests. Deliberately not defaulted to endUtc, which would restore the bug behind a nicer signature. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Put CancellationToken last (CA1068) Adding nowUtc after the token tripped CA1068 -- one new warning against a base branch that had zero, which is the check that catches this class of edit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…y knows (#2352) (#2358) The service computes the correct DACL and detects when the real one is wrong. What it lacks is authority: re-ACLing a file it does not own needs WRITE_DAC, and taking ownership needs a privilege a virtual service account is not granted. So it logs the remedy and carries on -- correctly, because a monitoring service must not refuse to monitor over a permissions problem -- and the only thing that ever APPLIED the rule to an existing install was install-darling.ps1. A box registered through the README's own sc create path left the operator hand-typing three icacls lines out of a CRITICAL log line (#2352). Seen on a real box today: darling.json and lp.secret both carried BUILTIN\Users:(RX) with inheritance unprotected, and every service start had been saying so, precisely and with the right commands, since the box was built. It VERIFIES rather than claims. Every target is re-read after the attempt and reported SECURED or STILL READABLE, and the exit code is 1 if anything is still exposed, so it can gate a provisioning script. "We tried" is not the same statement as "the secret is not readable" -- the distinction that let a permissions call which silently did nothing hide in the field for months. darling.json is the only target the interactive operator keeps read on: the Viewer and the CLI verbs run as that operator and must still read it. Nothing reads a backup (#1769), so the .bak-* siblings, the store directory and the DPAPI credentials get no interactive ACE. A config that will not load is a warning rather than a stop -- an unreadable darling.json is the very failure this repairs, so refusing to run without it would be refusing exactly when it is needed. Missing targets are skipped quietly: a BYO-Postgres install has no managed credential files and their absence is not a fault. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
#2347) (#2360) The .NET 10 SDK dropped VSTest-mode support for MTP-based frameworks, so xunit.v3 4.0.0 cannot be taken at all while `dotnet test` runs in VSTest mode -- it fails before a single test runs, and it blocks two Dependabot bumps (#2337, #2338) that are not themselves the problem (#2347). Deliberately done on the CURRENT xunit 3.2.2. The runner migration and the version bump are separate risks and deserve separate green runs; xunit.v3 has shipped MTP support since 1.x, so nothing here needs the new version. Once this is in, the bumps become ordinary. Both VSTest packages are gone rather than upgraded. xunit.runner.visualstudio exists only to bridge xunit to VSTest and Microsoft.NET.Test.Sdk IS the VSTest host, so under MTP they are not dependencies to update -- they are dependencies to delete. That closes #2337 as unnecessary rather than merging it. The invocation is `dotnet run --project`, not `dotnet test`. `dotnet test` in this SDK routes to VSTest unless opted in through global.json, and that file carries the SDK pin, which is not something a CI change should be reaching into. It also swallows arguments: with the MTP runner selected, EVERY argument passed after `--` produced "0 tests ran, exit 5" -- including a bare `-trx`. A test command that silently runs nothing and a test command that silently passes everything are the same catastrophe, so it is not used. The Installer filter was translated, not dropped. It excludes three suites that need a live SQL Server, and `FullyQualifiedName!~X` becomes xunit's own `-class- "Installer.Tests.X"`. Every claim above was verified locally before touching CI, which was possible because Installer.Tests targets plain net10.0 and therefore runs on macOS: - VSTest baseline with the old filter: 194 passed, 0 failed - MTP with the translated filter: 194 passed, 0 failed (exact match) - MTP with no filter: 214 total, 20 failed (the SQL-dependent suites) - exit code with failures: 1. exit code clean: 0. -trx writes the file and creates the directory, so the existing upload-on-failure steps keep working unchanged. The other three suites target net10.0-windows and cannot run here; they build clean with OutputType Exe, including the WPF ones, and CI is where they first execute. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…gible (#2359) (#2363) Server Inventory deliberately lists every REGISTERED server, and it joined the registry without ever reading is_enabled. So a server removed from monitoring kept the collection_time it had when collection stopped -- an accurate number that every operator reads as a broken freshness column (#2359). Reproduced on my own fleet, and the split is total: 19 disabled servers last collected within five minutes of each other on the day they were removed, 42 enabled servers all fresh within a minute of each other, zero overlap. Nothing was stale; nineteen things were finished. That also explains the reporter's "some (most)" with no visible pattern -- the pattern is which servers were decommissioned, which the grid never showed. The disabled rows are KEPT rather than filtered. This is the FinOps tab, and a decommissioned server's cost history is exactly what someone opens it to look at; dropping nineteen rows would trade a confusing grid for a lying one. They sort last, and a new Monitoring column reads Active or Stopped. "Stopped" rather than "Disabled" because the question being asked is what happened to the data, not what state a config row is in. The reader ordinals moved and are pinned by number: is_enabled lands at 17 and pushes monthly_cost_usd to 18. A positional reader does not fail on an inserted column -- it keeps reading one off and turns a cost into a boolean -- and that exact mistake shipped once already in the Aurora detection query. The new column carries no explicit Style (#2181/#2331): DarkButton and friends live in MainWindow.xaml's window resources, a scope this grid's templates cannot see, and a StaticResource reference there resolves at parse time and throws when the grid is realized. Not addressed here: the reporter also saw Status reading UNDER PROVISIONED. The verdict strings match exactly between ProvisioningVerdict and the UI, and a server with no recent metrics evaluates to OVER_PROVISIONED rather than UNDER, so it is neither a mismatch nor a consequence of this bug. Asked for detail rather than guessing. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…two (#2362) (#2365) The #2216 accumulator had two call sites -- blocking and deadlocks. Five other alerts produce dedup-keyed incidents and carried no total, so a consumer ends up with two permanent code paths: SET the field where it exists, GET-and-add on every recurrence where it does not, with a running number only as good as the deliveries it happened to see (#2362). The observation list is UNCAPPED and the card stays capped. That is the whole trap in this change. Each context builder renders 3 or 5 entries because a card with fifty helps nobody, and extracting the existing expression would have inherited that cap -- so any fingerprint falling out of the displayed top N would restart its count the next time it surfaced. That is the undercount #2216 exists to fix, reintroduced by the fix for it. BlockingIncidents already carried the reasoning; these five now share it, as pure functions of a list so the check passes the full set and the builder passes its capped subset. Observation sits OUTSIDE the fire branch for the same reason blocking's does: counting only at delivery lets an event that ages out during a cooldown mask an arrival. Two deliberate asymmetries, both verified rather than assumed: Forced Plan Failing is NOT included, though the issue listed it. CheckForcePlanFailuresAsync builds a bare AlertContext inline and never calls AlertIncidentRenderer.Apply, so it has no dedup keys for the accumulator to key on. Giving it a total needs fingerprinted incidents first, which is a design question about what the right fingerprint is. Anomalous Jobs IS included, though the issue did not list it -- there are two job alerts, not one, and both are fingerprinted on job name. Failed Agent Job observes but never clears, because it has no else-branch to clear from: a failed job is an event, not a condition that resolves. The accumulator's staleness horizon is the cleanup path there, which is the right shape for an event stream. The other four clear on resolution like blocking and deadlocks do. Reported by @gotqn, who supplied the call-site count and the reason the machinery was already generic. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eside it (#2359) (#2366) server_properties ships with FrequencyMinutes 0, which the schedule table defines as "collect once on server load only (config snapshots)". So Server Inventory's Last Updated was the last SERVICE START, and on an install that had been up a week every actively monitored server showed a week-old date. The value was always correct; the label invited the only reading anyone would give it (#2359). Renamed to Inventory As Of, and Last Collected now carries the real heartbeat - MAX(collection_time) from v_collection_log, the same signal list_servers and the Overview cards use. Both are shown rather than one replacing the other: a decommissioned server still needs its snapshot time, and #2363's Monitoring column is what makes that legible. Darling only. Lite has the same grid and the same property name, but populates it with DateTime.Now because it reads live on demand -- so its column genuinely means "just now" and was never misleading. Checked rather than assumed; the two ServerPropertyRow classes are separate per SKU, so this is not a parity gap. last_collection is appended LAST in the projection, at ordinal 19, so nothing that came before it moves. A positional reader does not fail on an inserted column, it keeps reading one off -- the Aurora detection query already shipped that mistake once. This is the bug @ghauan actually reported. My first pass diagnosed decommissioned servers showing stale timestamps, which was a real defect and is fixed in #2363, but it was not their case: they said plainly that none of their servers had been removed, and they were right. The correlation I measured (19 disabled stale, 42 enabled fresh) was an artifact of my own service restart 1h26m earlier -- those 42 timestamps spanned 15 seconds, which is one startup sweep, not independent collection. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
It reads raw query_store_stats, and on a store with the rollups armed that table is dropped at four days -- measured on the use2 fleet: a 4d 13h span over 17,162,516 rows under a policy_retention of "4 days". So a 30-day request returned at most four days, echoed hours_back: 720 back unchanged, and marked nothing (#2364). This cannot be routed to a rollup the way #2353 was, and the reason is structural rather than incidental: QueryStoreTopSql groups by database_name, query_id, plan_id, query_hash, replica_role, while the corrected CAGG groups by database_name, module_name, query_hash. No query_id, no plan_id -- and plan identity is the entire purpose of a tool whose own description is "pull the full Query Store entry including plan_id and forced-plan history before considering a force". A rollup grained to plan identity would approach the size of the raw data. So the fix is honesty, not routing. The window floor comes from a bounded MIN probe, never from the returned rows. That distinction is load-bearing: the result is the top N by COST, so its timestamps say nothing about how far back the read reached -- the most expensive query in a month may have run this morning. Deriving the window from the rows would give a confident wrong answer, which is worse than the silence it replaces. The probe is bounded on both sides so it prunes chunks and stops at the first row. The empty path no longer blames Query Store configuration alone. "Query Store may not be enabled" is a confident wrong diagnosis for a window that simply reached past retention, and an agent acts on it by going to inspect a configuration that is fine. Found while scoping #2357 with @carlei1978, who supplied the tool and window that made it findable. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
A consumer had to string-search Details[] for a section whose fields carry a Database label. That is exact only for deadlocks -- they are self-contained, built with includeDetailFields: true -- while every other fingerprinted alert appends a BARE Incident item beside its data item, so the incident's own section has no Database and the fallback degrades to "any Database anywhere in the payload". On a multi-incident alert spanning databases that is not an approximation; it is the wrong value with nothing marking it wrong (#2361). Database is a first-class member rather than a promoted DetailFields entry. It does not overlap InvolvedObjects: that is what the incident is ABOUT (tables, mount points, job names), this is its scope. Null when the alert is not database-scoped, because a disk and a job are not, and an empty string would read downstream as a database whose name is blank. The grouper's "unknown" sentinel becomes null too. It is fine as a display string and wrong as a data member -- a consumer routing on Database would file tickets against a database literally named "unknown". Deadlocks read theirs from the #2109 discrete fact on the representative's detail fields, which is the same lookup the consumer was doing by hand, done once where it is exact and where a multi-database graph's CSV survives intact. LastEventUtc is a projection, not a new measurement: IncidentOccurrenceState already carried LastObservedUtc as the value its staleness horizon compares against, and it simply never reached the incident. It rides the same OccurrenceTotals.Decorate hook that already attaches TotalOccurrences and IncidentStartedUtc -- which is why #2362 had to land first, so the field exists for seven alerts rather than two. Both members are trailing and optional so contextJson persisted before they existed still round-trips; alert history is durable and a deserialization failure would break reading alerts that were written correctly at the time. Both live in shared Notifications, so Lite and Darling get this by construction rather than by parity maintenance. Requested by @gotqn, whose diagnosis of the Apply-versus-BuildItem asymmetry is what made the current workaround's inaccuracy visible. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ant (#2357) (#2369) * Make the compose statement_timeout a store setting instead of a constant It is the hard backstop a composed query can never exceed -- a LIMIT bounds OUTPUT, a group-by scans and sorts before it, so something has to bound WORK -- and it shipped as a bare 15s. Fifteen seconds is a judgement about how big a store is and how fast its disk is, and the product knows neither for anyone else's deployment (#2357). Raising the constant would not have helped anyone already running. The value is applied in role PROVISIONING DDL, deliberately not a versioned migration, because a role's statement_timeout has no probeable schema footprint and tying it to the schema version would break the viewer's connect-time gate -- so an existing install has the old value baked into its roles and a new build alone changes nothing. What makes this deliverable without new machinery is that the same DDL is re-run on every managed start ("idempotent + self-healing: re-run every managed start, converging role state"). Reading the knob there means a changed value reaches a running store on its next restart. Ordering is what makes that safe: migrations run before provisioning at startup, so the column exists by the time it is read, and the read is defensive anyway -- a role-provisioning step that threw over a tuning knob would stop the service starting over something with a perfectly good default. Clamped to [5,600] in TWO places on purpose. The config read clamps, and BuildProvisioningSql clamps again because it is public and a caller passing 0 would remove the ceiling entirely -- the one outcome the whole design leans on not happening. Default 15 reproduces the constant exactly, so an untouched install is byte-identical. V78 carries the column. The rung recipe is complete: migration + StorageVersion + probe sentinel + reader ordinal + newest-first arm, with V77's "I am the top" claim demoted to the invariants that stay true forever. Reported by @carlei1978, who hit it on a 30-day get_query_store_top -- a window that, as #2364 established, this tool genuinely has to serve from raw. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Derive the older rungs' InvokeMap padding from the map's arity CI caught three rung tests failing "Expected: 54, Actual: 53" -- V74, V75 and V76 each build their argument list by hand, appending one literal false per rung added after theirs, so every new rung breaks all of them. V78 was the fourth to do so. Padding is now computed from the method's arity, which keeps the intent exactly (later sentinels FALSE, so each fact still exercises its own arm rather than a newer one) and means the next rung does not have to edit these files at all. The failure could not surface locally: the suites target net10.0-windows and the assertions are runtime, not compile-time, so the projects built clean on macOS while three tests were already wrong. CI is the only place this shows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Alert on database file size growth, graded per server Between tempdb Space and Volume Free Space sits a file that has grown large but has not yet filled its disk, and neither existing alert can express it (#2349). tempdb Space fires on reserved / (reserved + unallocated) -- autogrowth adds unallocated extents, so the denominator grows with the file and the percentage FALLS as tempdb balloons. Volume Free Space fires on the consequence, by which point a restart is already overdue, and cannot attribute the space to any one file. Two gates, both graded per server, because config_alert_settings is a single global row and an absolute MB threshold is unusable across a fleet whose normal tempdb sizes differ by an order of magnitude: set it low enough for the small instances and the large ones alert constantly. The RISE gate is primary. #2157's reasoning applies exactly -- a level alone re-pages every cooldown about a size that has been true since Tuesday, which trains people to mute it, while "80 GB in the last hour" is the event. The LEVEL gate is the file as a share of its VOLUME, which self-scales to each server's disk layout whether or not the file has a dedicated one, and catches the file that is already large and has stopped moving -- the state the rise gate goes quiet about by design. Zero disables either gate independently, so rise-only or level-only needs no second switch. Both gates come from ONE read. The store already holds the time series, so the rise is measured against a baseline sample from inside the window rather than tracked in memory -- no per-file state to keep, persist, or expire. The window width is MEASURED rather than assumed, so a gap in collection cannot make a slow rise look fast, and a window holding a single sample reports no rise rather than a rise of the whole file. Fingerprinted per FILE. Eight tempdb data files growing together are eight files and one problem, but a log file running away while its data files sit still is a different incident, and collapsing on database name would merge the two and pool their totals. The observation list is uncapped per #2362. Both SKUs: DISTINCT ON on Postgres, ROW_NUMBER() on DuckDB, same rule. Ships OFF, because a new alert that starts firing on upgrade is a bad citizen and the right thresholds are a property of the fleet rather than of the product. Requested by @gotqn, whose analysis of why tempdb Space cannot answer this was exact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Derive rung-test map padding from arity, not by hand V79 broke ActivityDrivenPlanFetchStoreTests, which named both trailing parameters explicitly and asserted the count matched: 54 args against a 55-parameter map. Fixed the same way the V74-V76 tests already were -- pad from the method's arity so a new rung needs no edit here. Also corrected V78's and V79's own helpers, which derived the LEADING count from arity instead. That reads identically while a rung is at the top of the ladder, then slides its flag one position right per rung added above it, so the assertion keeps passing while testing a newer arm. Each now pins its own ordinal (52/53/54, verified against the signature) and derives only the trailing pad. Refs #2349 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
--harden-files resolved the account to grant from WindowsIdentity.GetCurrent(), so running it the documented way -- elevated, from something that is not the service -- wrote the ACL for the operator and STRIPPED the account the service runs as. Measured on a live box: an elevated run removed NT SERVICE\PerformanceMonitor Darling from all four targets and printed "All 4 item(s) secured" while doing it. The service kept running on its open handles and would have failed on the next start, unable to read darling.json or either DPAPI credential -- the failure #2185/#2197 exist to diagnose, manufactured by the tool meant to prevent it. Every original caller of DarlingFileSecurity runs INSIDE the service, so the current identity and the service account were the same value and the distinction did not exist. This verb inverts that by construction: it exists because a virtual service account cannot re-ACL a file it does not own, so its caller is never the service. The account now comes from the SCM's ObjectName, which also covers the re-homed domain-account and gMSA cases the property's own doc comment was already worried about, and falls back to the caller when the service is not registered (console run, or hardening before install). The verify pass gains the other half. IsReadableByOrdinaryUsers asks whether anyone too many can read; it cannot see the opposite failure, and an ACL excluding ordinary users AND the service is maximally private and completely broken. That is why the live run reported success on four locked-out targets. A lockout now reports LOCKED OUT and exits non-zero. Fixes #2371
…#2375) DatabaseStateExpectedStoreTests failed a nightly on Assert.Single() against an empty collection, three lines after a sweep that quietly did nothing. Same tree passed before and after, so it is timing. The baseline seed rides GetDatabaseStateDeviationsAsync's BEST-EFFORT maintenance block: it takes the write connection with a 5-second lock acquisition and, on TimeoutException, skips seed/heal/forget/prune and runs the deviation read anyway. That is deliberate and documented -- skipping is the only lossless option when archival holds the lock. The lock is static and shared by the whole process, xunit runs test classes in parallel, and the method's own comment already records this exact interaction biting once before. So a bare sweep can return having written no expectation, and a test that then acts as though it has a baseline is asserting on a precondition it never established. It fails far from the cause, because the read still succeeds and simply has nothing to deviate FROM. SweepUntilAsync already solved this one step later, for the #2189 heal, with the same reasoning. BaselineAsync is that helper for the seed: it settles on the recorded STATE, not on row count, because GetDatabaseStateExpectationsAsync LEFT JOINs the newest snapshot and so returns a row for every current database whether or not anything was seeded -- an unseeded one is a present row with an empty expected state, making "did I get rows?" always true and never the question. Re-reading it retries the seed, since it re-runs that side effect itself. The pending-states test gains the most. A skipped block seeds nothing, which reads as all three databases pending -- so its two "" assertions were passing for the wrong reason and only Healthy reported the problem. Settling on Healthy first proves the seed ran before asking what it declined to learn. Cannot mask a regression, for SweepUntilAsync's reason: a seed that is genuinely broken never records the state, every cycle runs, and the caller's own assertion fails on the same empty result it sees today. Fixes #2374
It is the worst-compressing table in the store -- 4.5x measured, against siblings at 9.6-36.3x -- and the second largest before compression. Compression delta-encodes each column down a batch of rows, and the default order is the partition time descending. This table writes thousands of distinct queries at a single collection time, so consecutive rows in a batch are unrelated queries and the encoder differences one query's metrics against a different query's. That is noise, and it is why the widest metric row in the store compresses worst. Measured on TimescaleDB 2.28.1 / PG 18.4 -- the deployed pair, not a newer local one -- over 240,000 rows of the real 41-bigint shape at 594 bytes/row against production's 576: seg=server_id, NO orderby (shipped) 57 MB 2.36x seg=server_id, ord=collection_time,query_id 56 MB 2.43x seg=server_id, ord=query_id,collection_time 20 MB 6.89x seg=server_id, ord=query_id,plan_id,time 20 MB 6.88x seg=server_id+database_name, ord=query_id.. 20 MB 6.90x The time-first control is what makes this a finding rather than a guess: restating the default buys nothing, so the effect is entirely about the leading column. plan_id and a second segment-by add nothing, so the minimum is also the maximum. Settings bind FUTURE compressions only. Verified on 2.28.1, because the statement re-runs on every managed start and an error there would take out every existing install: against a store whose chunks are already compressed under the old settings the ALTER is a NOTICE and succeeds, is idempotent across repeat runs, old chunks keep their layout and stay readable, and the next chunk compresses under the new order. Measured side by side on one table: old chunk 4048 kB, new chunk 2152 kB for the same rows. Recompressing the backlog stays manual. ONE table deliberately. query_stats keys on query_hash and procedure_stats on sql_handle, neither has query_id, and both already sit near 9.5x -- so they are plausible candidates and are absent, because extrapolating a measurement of one row shape onto a different one is the guess this finding replaced. Refs #2316
16 entries move section-for-section (5 Added, 3 Changed, 8 Fixed) to the top of the existing [3.5.0] block, newest first. Bullet count unchanged at 919 and CRLF preserved.
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
1 participant
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.
Release 3.5.0. 25 commits,
dev@bf7c3ab5.Version check will go RED, deliberately
check-version-bumpcompares the PR's version against main's and both read3.5.0, because main was stamped 3.5.0 by the earlier dev→main merge (da5c5aa9) that was pulled back and never tagged. The gate is firing on a stale stamp, not a real problem:3.5.0## [3.5.0] - 2026-08-19It is not a required check (branch protection requires
buildandDarling PostgreSQL tests), so it does not block. Bumping to 3.5.1 or 3.6.0 purely to satisfy it would invent a version to appease a false positive.What is in it
The
[Unreleased]block was folded into[3.5.0]in #2380 — 16 entries, section-for-section, bullet count verified unchanged.Landed today, all verified live on the three-box fleet:
statement_timeoutas a store setting (V78)Databaseand a last-event timestamp onAlertIncidentDto#2361 / [FEATURE] Extend the #2216 occurrence accumulator to every fingerprinted alert, not just blocking and deadlocks #2362Database+LastEventUtcon incidents, and occurrence totals on five more alertsget_query_store_topandget_query_trendreport the window they actually served--harden-files, plus --harden-files strips the service account it is meant to grant, because it resolves the account from the caller #2371 — the verb resolved the service account from the CALLER, so running it as documented strippedNT SERVICE\PerformanceMonitor Darlingfrom all four targets while printing success. Found by running it on a real box; fixed and re-verified there.Verified on the fleet, not just in CI
All three boxes are running this exact tree (
a00c9aa7plus the CHANGELOG fold), schema v79, zero collection errors:Behaviour confirmed against 42 live servers rather than fixtures: #2364's empty path names the window and stops blaming configuration; #2353 routes to the hourly rollup and reports
source/effective_hours_back/bucket, nulling what the rollup does not carry instead of zeroing it; #2339 discloses coverage correctly with nothing declared. #2316's dedicated plan-content horizon also fired for the first time in production — 407,167 rows in one sweep, dimension pinned to its horizon to the minute.What is NOT in it
#2377, the
query_store_statscompression order-by, was merged and then reverted (#2379). On a store with chunks already compressed under the old settings, recompression fails on a bloom/minmax sparse-index conflict, andquery_store_backfill_enabledis true in production — so the automatic recompression path is reachable. The measurement survives on #2316 (2.36x → 6.89x) for a future attempt with bloom handling.Known caveat
The soak on this exact build is short. Today's fixes each reset the clock, and the release is being cut on the vendor's call rather than after an overnight run.