3.5.1 release prep: version bump and changelog cut - #2395
3.5.1 release prep: version bump and changelog cut#2395erikdarlingdata wants to merge 5 commits into
Conversation
Versions move to 3.5.1 in the three live csprojs plus the deprecated Dashboard csproj the dev->main version check reads. 3.5.1 exists because two of the three fixes it carries are defects 3.5.0 introduced, and neither produces a signal. #2388's retention delete could time out and roll back to zero rows, or -- worse -- succeed while removing a fraction of what expired, logging "0 failed" the whole time; measured on the dogfood store it was deleting about 6% of what crossed the horizon and falling ~360k rows/day behind. #2390's analysis query read the entire query_store_stats history every cycle because it never bounded the partitioning column. A store hitting either one finds out when a disk fills, which is not a discovery path worth leaving in a shipped release. #2391 rides along because it is the same shape at smaller scale: an alert that ships OFF with no supported way to turn it on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed. This is a pure release-prep PR (version bump + changelog) — no T-SQL, no application code, so the usual style/parity/security/perf checks don't have anything to bite on. Verified the mechanics:
No findings — nothing to flag on correctness, Lite/Darling parity, security, or performance grounds for this diff. |
|
Putting this back to draft — I cut it before validating the thing it exists to fix, which is the same gap that let #2386 and #2387 ship in 3.5.0 in the first place. #2388's batched drain has never run against real data. Its coverage is a fake-executor loop test plus a string assertion on the SQL; the two have never met a real planner together. #2397 adds that (110 expired rows, cap 25, asserts the total and the batch count), and it runs under Gate for taking this out of draft:
The measurement to watch is |
The 3.5.1 entry credited #2385 with fixing the comment. #2385 replaced one wrong claim with another -- readers do not queue behind each other on a ReaderWriterLockSlim -- and #2396 is what makes it accurate. Worth saying in the entry rather than quietly citing the corrected version, because the distinction is the part that tells a reader a collection fixture would make it worse. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| - **The file-growth alert's four knobs reach a user on all three surfaces** ([#2391], [#2349]) - #2349 shipped the Database File Growth alert **OFF** with its knobs wired through the store and the alerting engine and exposed nowhere: no MCP group, no Settings control on either SKU. For an alert that ships off, that made a hand-written `UPDATE` against `config_alert_settings` the only supported way to enable a feature that had been requested. `get_alert_settings` now reports a `file_growth` group and `update_alert_settings` accepts `enabled` / `rise_mb` / `volume_percent` / `lookback_minutes`; both Settings windows carry the control row, including the `Will alert when:` summary, which previously could omit an alert that was enabled and able to fire. Write bounds match the engine's clamps exactly, pinned by a test that reads both files, so no surface can accept a value the engine silently rewrites on read - and zero stays valid on the rise and volume gates, because #2349 uses zero to disable a gate and a floor of 1 would have removed the rise-only and level-only configurations. The service side needed no change: `StoreConfigProvider` has selected and read all four columns since #2349. Reported by @gotqn. | ||
|
|
||
| ### Changed | ||
| - **The DuckDB test fixture's doc comment now describes what class-per-database actually buys** ([#2385], [#2396], [#2376]) - it claimed the `IClassFixture` choice kept xUnit's cross-class parallelism intact, which `DuckDbInitializer`'s process-wide lock does not permit. The file separation is real and worth having; the parallelism claim was not, and a comment asserting a property the code does not hold is worse than no comment when the next person is deciding whether to change the topology. The first correction then overshot in the other direction - it said every read and write queues on one lock, which is not what a `ReaderWriterLockSlim` does: readers do not queue behind each other, and the suite is overwhelmingly readers (~184 read call sites against ~20 write sites), so the contention is writer-driven and bursty around archival, compaction and CHECKPOINT rather than a flat tax. That distinction decides what a fix could look like, since a collection fixture would serialize the readers too and cannot help writer-driven contention. |
There was a problem hiding this comment.
[#2376] is referenced here but has no corresponding link definition added to the reference list at the bottom of the file (the other seven new references — #2384, #2385, #2386, #2388, #2390, #2391, #2396 — all got one; #2349 already had one from the 3.5.0 entry). As-is this will render as literal [#2376] text instead of a link.
| - **The DuckDB test fixture's doc comment now describes what class-per-database actually buys** ([#2385], [#2396], [#2376]) - it claimed the `IClassFixture` choice kept xUnit's cross-class parallelism intact, which `DuckDbInitializer`'s process-wide lock does not permit. The file separation is real and worth having; the parallelism claim was not, and a comment asserting a property the code does not hold is worse than no comment when the next person is deciding whether to change the topology. The first correction then overshot in the other direction - it said every read and write queues on one lock, which is not what a `ReaderWriterLockSlim` does: readers do not queue behind each other, and the suite is overwhelmingly readers (~184 read call sites against ~20 write sites), so the contention is writer-driven and bursty around archival, compaction and CHECKPOINT rather than a flat tax. That distinction decides what a fix could look like, since a collection fixture would serialize the readers too and cannot help writer-driven contention. | |
| - **The DuckDB test fixture's doc comment now describes what class-per-database actually buys** ([#2385], [#2396], [#2376]) - it claimed the `IClassFixture` choice kept xUnit's cross-class parallelism intact, which `DuckDbInitializer`'s process-wide lock does not permit. The file separation is real and worth having; the parallelism claim was not, and a comment asserting a property the code does not hold is worse than no comment when the next person is deciding whether to change the topology. The first correction then overshot in the other direction - it said every read and write queues on one lock, which is not what a `ReaderWriterLockSlim` does: readers do not queue behind each other, and the suite is overwhelmingly readers (~184 read call sites against ~20 write sites), so the contention is writer-driven and bursty around archival, compaction and CHECKPOINT rather than a flat tax. That distinction decides what a fix could look like, since a collection fixture would serialize the readers too and cannot help writer-driven contention. | |
| [#2376]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2376 |
|
Reviewed. This is a low-risk, mechanical release-prep PR (version bump + changelog cut, no application or T-SQL code) — verified:
No Lite/Darling parity concerns (both apps' versions move together), no security-relevant surface touched. |
Both were found by asking what in this release is verified by anything other than reading the diff. The checksum fix was half-applied (#2384 fixed the release workflow, not the nightly), and the dispatch-ref bug meant the release candidate I built to soak was not the branch I dispatched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both gate conditions are met, so this is out of draft. 1. #2397 green — the live drain test ran against real PostgreSQL (confirmed executed, not skipped: 2. Verified on the box in the failure state. Against four consecutive runs on 08-19 that hit ~301 s and deleted zero rows each. Afterwards: 0 rows past 23 days, oldest surviving Scope stays as cut, deliberately
So this ships what it says it ships. Every item in it — #2388, #2390, #2391, #2384, #2385 — is in the build currently soaking on all three monitor boxes, so the contents are validated rather than merely merged. One correction to my own gate note above: I wrote that the 1.16 M figure was the backlog. It was measured against |
ServerConnection.DotStatus computed "Unknown"/"Online"/"Warning"/"Offline" from its own (IsOnline, HasCollectorErrors) pair -- the same four-word ladder the Overview card derives, on a different type, on a different surface, from a different instance of the same flags. The two could say different things about one server and nothing would notice. That is the drift #2429 collapsed the viewer's card for and #2451 collapsed Lite's, and this is the last of them in Lite. Not the last of them anywhere, and an earlier draft of this message said so. Review caught it: DarlingServer.DotStatus in the Darling viewer is a fifth copy, and unlike this one it is not merely capable of disagreeing with its card -- on dev it already does. Both surfaces there classify the SAME ServerFreshness, and a never-collected server draws a grey "Unknown" dot beside an amber "Awaiting first collection" card. That is a different app, a different enum and a different vocabulary decision, so it is filed rather than folded in here. The collapse is not "ServerConnection switches on the pair too, but returns an enum". A second switch is a second ladder however it is typed, so the ladder itself moved into one function: ServerCardStatusRules.Classify, with Word() and Headline() beside it. ServerSummaryItem.CardStatus and ServerConnection.CardStatus both call it, and StatusDisplay/DotStatus and StatusHeadline/DotTooltip are renderings of the result. The card's brushes stay where they are -- hex colours are the card's business and the sidebar paints from theme resources -- but they still switch on CardStatus, not on the flags. The dot gets the tooltip the card's dot got in #2451, opening on the same sentence word for word. It is the thing a reader points at first, which is #2422's complaint one surface over from where it was reported. Its middle line is the part this surface specifically needs. #2457 deliberately kept collection freshness OUT of the status word and gave it its own banded row on the card; the sidebar has no such row and ServerConnection carries no last-collection time to build one from, so a green dot here is a connection answer read somewhere that offers no freshness answer at all. The tooltip says so and names where the freshness answer lives, rather than leaving a reader to infer one from the other -- which is the #2429/#2422 conflation in miniature. Classify takes two arguments and a test names that signature, so folding freshness in cannot happen by accident. #2451's pin moved with the ladder rather than being loosened, and it got wider, because the way it missed this copy is instructive. Assert.Equal(1, CountOccurrences(source, "IsOnline switch")) counted one literal in one file. The fourth copy was in another file AND was written as a chain of if statements rather than a switch, so it evaded that pin on both axes at once and went on evading it through #2451 and #2457. The count now spans both files and pins the classifier's own parameter list; and a new assertion holds the invariant the count was standing in for, syntax-agnostically -- the four words are string literals that appear in exactly one function, and none of them is written in ServerConnection.cs any more. Verification. Lite.Tests targets net10.0-windows and cannot run on macOS, so the logic was run for real anyway: a throwaway net10.0 harness splices ServerCardStatus, ServerCardStatusRules, ServerSummaryItem and the ServerConnection members straight out of the shipped files (a generator reads the source and emits them -- never retyped) behind a System.Windows.Media shim, with a real project reference to PerformanceMonitor.Common. All 35 checks pass, including every reachable (IsOnline, HasCollectorErrors) combination agreeing between the two surfaces. Against dev the strongly-typed harness does not compile at all, so the comparison is run as the identically-expressible subset with the new members reached reflectively: 7 passed / 6 failed on dev, 13 / 0 on the branch. The 7 that pass on both are the controls, and they are the ones a wrong fix breaks first -- the two ladders agree TODAY, which is why this defect is drift potential rather than a live disagreement, and why the assertions that separate the branches are about construction rather than output. The source- and XAML-scanning half was simulated in Python against both versions: 13 fail on dev and all 18 pass on the branch. Whole solution builds, 0 errors. CHANGELOG deliberately untouched -- #2395 is an open release-prep PR that owns that file for 3.5.1. One thing noted and not fixed here: Lite/Mcp/McpDiscoveryTools.cs renders a THIRD vocabulary for the same axis -- "Online"/"Offline"/"Status not checked" -- off ConnectionStatus.IsOnline. It is a different type, it never reads collector health so it has no "Warning" state, and the MCP text is a consumer API, so changing its words is a decision rather than a cleanup. Filed as a separate concern rather than folded in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Version bump to 3.5.1 across the three live csprojs plus the deprecated Dashboard csproj that the dev→main version check reads, and the changelog cut.
3.5.1 is a patch release for two defects 3.5.0 itself introduced, both of which are silent:
DELETEby a time slice rather than by rows. Slice width is set by data at the old end, not by how far behind retention is, so the statement could exceed the 300s timeout and roll back to zero rows deleted — or, worse, succeed on a thin slice and log0 failedwhile the table grew. Measured on the dogfood store: deleting ~6% of what crossed the horizon, falling ~360k rows/day behind, 1.16M expired rows accumulated. Evidence on Tightening plan_content_retention_days permanently stops retention on query_plan_dim (one-day slice exceeds the 300s delete timeout) #2386.query_store_statshistory.Plus #2391 (file-growth knobs reachable on all three surfaces), #2384 (SHA256SUMS written with LF so
shasum -cworks), and #2385 (fixture doc comment corrected).check-versionis expected RED on this PR — it compares against main and only goes green once dev→main merges. That is the same behavior as the 3.5.0 cut.