[pull] main from erikdarlingdata:main - #21
Merged
Conversation
…tence Assert the retention-horizon convergence moves nothing once settled
…onitor into fix/2874-straggler-timeouts # Conflicts: # CHANGELOG.md # Darling/PerformanceMonitor.Darling.Service/ServiceCommandDeadlines.cs
CHANGELOG.md is taken verbatim from origin/dev. The three `### Fixed` bullets this branch carried are handed to the coordinator, who is collating every lane's entries into one change; no lane edits this file now, which is what stops five PRs serialising on it. The text is not lost by dropping it here: it is reported in full, and it stays recoverable from this branch's history at c28d240. This is the reverse of what 4213362 did. That commit deleted the same three lines claiming they had already moved, and they had not -- they were absent from dev, from every other open pull request and from every remote branch. They are being dropped now because a destination exists.
DarlingRetention.PurgeOneAsync lifts a TimescaleDB decompression rail through an
untimed `using (...)` whose single body statement let the timed delete behind it
stand in, so it inherited Npgsql's 30 s default. It now carries
DeleteTimeoutSeconds. It is the only such site in the repository and it sits
outside every pin's scan scope, so no pin's published figure was false - but the
class was live in shipped code.
Three independent blind spots let it hide, all of them a scan answering about
something other than the construction it points at.
StatementSpanFrom let its bracket depth go negative and keep counting, so a
construction in a `using (...) { }` with a one-statement body spent the window on
that statement and on the statement after the block. The span now ends with the
scope it started in - a closing BRACE ends it, a parenthesis does not - and still
follows a header into the block or the braceless statement it governs, which
fourteen sites need.
Two layouts no scope bound can reach, an untimed construction directly ahead of a
timed one and an untimed header whose block opens with a timed sibling, are closed
by splitting the question: ConstructionSpanFrom covers the site and its own
initializer, and only a member assignment is read from the statement span.
Truncating the span at the next construction instead breaks the conditional in
ViewerDataService.FinOps.Locking.cs, where two constructions share one deadline.
Both halves are now read over STRIPPED source, so neither a construction named in
prose nor a deadline merely spelled in a comment can decide a verdict. The
.Analysis and PgFactCollector pins move off a raw three-line window onto the same
rule, and all five take the alert pass's fully-qualified construction pattern.
Every scope re-measured and unchanged: .Storage 124/124, .Viewer 193/193,
.Analysis 73/73, PgFactCollector 32/32, alert pass 45/45, MCP reads 126/126.
…-timeouts # Conflicts: # CHANGELOG.md # Darling/PerformanceMonitor.Darling.Service/ServiceCommandDeadlines.cs
…darlingdata/PerformanceMonitor into fix/2874-straggler-timeouts # Conflicts: # CHANGELOG.md
…erikdarlingdata/PerformanceMonitor into fix/2874-command-plane-timeouts # Conflicts: # CHANGELOG.md
…ualified-shape Part of #2874
…window Part of #2874
…2874) Review caught that `borrowsFromStore` was computed once over the whole member body and then applied to every construction in it. A member that borrows from the store pool in one block and opens a monitored target into the same name in another would have had the store's open vouch for the target command -- the store-versus-target misclassification this pin exists to prevent, one granularity finer than the member scoping that closed it across files. It asks the NEAREST PRECEDING binding of `connection` instead. No preceding binding answers no, so a `connection` arriving as a parameter fails asking for a decision rather than being assumed. Latent rather than live: no current member rebinds that name, and the census is unchanged at 13. Proved red first with the reviewed scenario as a fixture -- it reads store under the member-wide answer and target under the nearest-binding one -- and the mutation that drops the precondition entirely now fails on that fixture and on the real target-read shape, separately.
cpu_utilization_stats is partitioned on collection_time, so ordering the latest-sample reads on sample_time - an unindexed payload column - appends every chunk and top-N sorts a server's whole retained history to return one row. Ordering on the dimension with sample_time as the within-batch tiebreak earns TimescaleDB's ordered ChunkAppend, which stops at the newest chunk. Deliberately without a time predicate: sample_time is the monitored server's local wall clock on the ring-buffer arm and naive UTC on the Azure SQL DB arm, so bounding it against store time returns zero rows - read as "no CPU data" rather than as an error - for every server behind the store's clock. Also corrects ViewerServerTab.Memory's frame note, which still described the memory-pressure sample_time as server-local after the collector moved to SYSUTCDATETIME().
…hout Both sides of the ServiceCommandDeadlines.cs merge continue a shared /// <summary> that sits above the conflict hunk, so keeping both sides left the second block's doc comment with a closing tag and no opening one -- 8 openers against 9 closers. DocCommentHygieneTests did not catch it: that rule counts two stacked <summary> OPENINGS per doc run, so a MISSING opening satisfies it vacuously. CI was green on a real doc defect, which is the pass-for-the-wrong-reason case this sweep keeps finding. Balance is now 9/9 and every constant sits outside an open element.
…-timeouts # Conflicts: # Darling/PerformanceMonitor.Darling.Service/ServiceCommandDeadlines.cs
…ed green CI Four groups append constants to ServiceCommandDeadlines.cs, and each group's block continues a <summary> its predecessor opened ABOVE the conflict hunk -- so a keep-both-sides resolution leaves the second block with a closing tag and no opening one. Group E hit it and reopened the element; this group hit it and shipped it: all six checks passed on a file with 8 openings against 9 closings. DocCommentHygieneTests cannot see it. That rule counts <summary> OPENINGS per doc run and fails on two or more, which is right for the displaced-block defect it was written for; a run with ZERO openings satisfies it vacuously. Proven on the same mutation -- this guard goes red naming the nesting depth, that rule stays green. Widening the shipped rule to require balance finds exactly ONE other offender across 2,108 .cs files (Lite.Tests/FindingStoreTests.cs:656, the identical opens=0 closes=1 shape), so the repo-wide version is worth adding and is tracked separately rather than folded into a deadline change. This is the narrow version: the one file where the trap is structural, and no cross-project surface. Also routes the nine sites through #2938's shared CommandDeadlineScanner as a second, additive guard. It answers a weaker question than the relational check -- 'is there a deadline' rather than 'is it this regime's constant' -- but answers it better, reading the construction's initializer separately and qualifying the assignment by the name bound to the construction, so a sibling's deadline cannot be borrowed. Proven to catch what the relational check alone accepts: a deadline attributed to an alias of the command fails only the scanner guard.
…e new live teardowns through LiveStoreCleanup ViewerOverviewSqlTests and DarlingMcpHealthToolsSurfaceAndSqlTests each pinned "ORDER BY sample_time DESC" on the read they cover. LiveCleanupConversionRatchetTests requires live-test teardown to run on its own freshly-opened connection (#1794/#1902): the body's connection is the one thing the failure being reported may have destroyed, and a throw from finally replaces the body's exception with connection noise.
…-binding-scope Part of #2874
…uard to see it DarlingFleetReader.FleetCpuSql is the fourth latest-CPU read and the only one with no server_id filter. It led on sample_time while FleetMemorySql directly below it already led on collection_time, so it was the odd one out in its own file and the standing invitation to bound a local-clock column across the whole fleet at once. The ordering change buys no speed there and the doc says so: DISTINCT ON with server_id leading and no per-server LIMIT reads and sorts the whole relation whatever the time key is (measured identical either way). A per-server lateral would be the cost fix, and that is a different query. The guard keyed on LIMIT 1 with a lone $1, which the fleet read has neither of, so the copy the guard could not see already existed under a guard whose own doc worried about a fourth copy arriving. The extraction now recognises the DISTINCT ON (server_id) shape, spans backward to the start of the literal so the select list is in scope, and rejects sample_time behind a leading server_id.
…timeouts Part of #2874
…d-comments Part of #2936
Order the latest-CPU reads on the hypertable's partition column instead of sample_time
/api/ping answered a hardcoded "ok" with no store touch and no runtime state, so it reported healthy while collection had never started. Every other health surface reads the store to find out, which is why none of them can report that the store is the problem. CollectorRuntimeState is the third seam of the McpRuntimeState / WebRuntimeState family: the worker publishes its own startup verdict, the web host reads it, and neither touches the store to do so. Every collection-blocking exit in the worker's startup path publishes before it returns - those returns complete the worker task successfully, so the host stays up, both Kestrel hosts keep serving, and on Windows the service keeps reporting Running. /api/ping now answers 200 ok (collecting), 200 starting (no verdict yet), 503 degraded (a startup step failing on #2936's retry budget, with the step, attempt and reason) or 503 stopped (a terminal stand-down, with the reason the critical log line gives). The healthy body still carries status: ok, so an existing check keeps passing and only stops passing when collection genuinely is not running. The census pin is source-parsed over a self-anchoring region - ExecuteAsync's body and RunCollectionLoopAsync up to the publish that says collection began - so a startup step added later joins the pin the day it is written.
…esolve on #3251 Resolutions, all add-vs-add beside this branch's changes: DarlingWorker keeps BOTH #3239's log-reader helpers/arm and this branch's declared-extension helpers/arm (the two when-guards cannot collide - the log readers declare no extensions); PostgresFaultOutcomeTests keeps both sides' new assertions and #3239's two new tests; the runbook's step-10 table keeps #3239's log-reader row and this branch's EXTENSION_MISSING row, dropping the stale "view/function absent -> PERMISSIONS" row that this branch reclassifies. CHANGELOG.md stays byte-equal to origin/dev per the wave protocol. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtjWJJ3NnSkDSrPBsGywoZ
) #2643's sibling arm inserts rows carrying only database_name, file_type_desc, file_name, total_size_mb and state_desc, because sys.resource_stats has per-database sizes and no per-file breakdown -- and ReadAsync read exactly the omitted ordinals (database_id, file_id, physical_name) unguarded. The first sibling row, which appears once sys.resource_stats finishes ingesting (roughly an hour after database creation, which is why every fresh-server validation window missed it), threw "Object cannot be cast from DBNull to other types." and aborted the whole read, master's own file rows included, permanently. - Guard ordinals 1, 2 and 5 and make Row.DatabaseId / FileId / PhysicalName nullable. WritePayload is unchanged: both row writers already carry null-writing int?/string? overloads. - Lite's DuckDB store held the three columns NOT NULL, so the fixed reader would only have moved the crash from the read to the append: drop the constraints in the generator overlay, record the deliberate divergence in the equivalence ledger (the #1591 pattern), and migrate existing stores at schema v57 -- dropping idx_database_size_stats_time first, the #2748 ALTER COLUMN dependency trap. Darling's Postgres store was always nullable here (verified against the pinned migration-ladder DDL); no migration. - The has_user_databases inventory screen (Lite CollectionHealth, Darling viewer twin, Darling MCP reader) admits sibling rows through an IS NULL arm: on a master-connected Azure target they are the only evidence of user databases, and database_id > 4 alone read such a target as having none. - Seam tests drive ReadAsync over the sibling shape the arm's own tests always pinned (proven red against the unfixed reader), plus an aged- database migration test through a real DuckDB upgrade and sibling- inventory tests on both engines' health probes. Every other consumer of database_size_stats was swept for NULL tolerance: the name-map readers already skip NULL ids, the analysis facts filter on is_percent_growth, and the FinOps/MCP/compose surfaces aggregate by name with guarded reads. Closes #3262 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtjWJJ3NnSkDSrPBsGywoZ
…stat_statements lands in PERMISSIONS The line-714 paragraph still walked the 42P01 example into the PERMISSIONS bucket, which #3240 in this same PR makes false - that case now records EXTENSION_MISSING naming the extension. Caught by fix-3247's cross-check; the three set-pinned spans in the extensions paragraph are untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtjWJJ3NnSkDSrPBsGywoZ
…ents (PROSE) The #3261 description rewrite states the #2625 dual-source truth, which puts an Aurora-only surface name into DarlingMcpPgStatementTools.cs for the first time — inside a string literal, exactly where the whole-tree guard looks. Accounted as PROSE, the CollectorEngineCapability precedent: the tool reads the store and only names the surface in its message; the dependency itself stays PgStatementStatsCollector's PAIRED entry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtjWJJ3NnSkDSrPBsGywoZ
…ondition Classify an absent declared PostgreSQL extension as EXTENSION_MISSING, not PERMISSIONS
…null Read the Azure sibling rows the size collector deliberately emits (#3262)
Complete the Darling README's PostgreSQL collector inventory (27 of 27), pin it, and retire the last seven-collector-era prose
…3243, #3244, #3245, #3247, #3248, #3253, #3261, #3262) The wave's fix PRs deliberately carried no CHANGELOG edits (parallel-agent hot-spot protocol); each agent reported its entry and this commit lands them together, byte-verified against origin/dev. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtjWJJ3NnSkDSrPBsGywoZ
…lice Add the fix wave's ten CHANGELOG entries in one consolidated splice
get_fleet_overview banded 14 healthy Aurora targets Warning in one tick: the collection_stale flag compared MAX(collection_time) against a flat 2-minute threshold tuned for 1-minute collectors, so a target whose metric-driving collector (pg_cpu_utilization) legitimately runs every 5 minutes read stale on any snapshot landing in the back half of its window - with every collector on the server on schedule. The stale cutoff is now per server: a target is stale only when every enabled scheduled collector is overdue against ITS OWN effective cadence (twice the cadence, floored at the flat 2 minutes - the same max(floor, multiplier x cadence) shape #1573 gave the per-collector bands). ServerHealthClassifier.EffectiveStaleThreshold reduces the per-collector (newest run, cadence) samples to one threshold; cadence resolves store overrides over CollectorScheduleDefaults (per-server > fleet-wide > default), so the band honors the schedule the sweep actually runs. The Offline band stays flat at 30 minutes (#2794's contract with the alert engine) and is checked first, so a genuinely dark server still bands on schedule. Wired everywhere the band is decided, so no two surfaces can disagree about one server (#2473): the Darling fleet reader (get_fleet_overview + /api/fleet), list_servers, the WPF viewer's Overview cards and sidebar dots, and Lite's Overview card - whose shipped Low-Impact preset runs every collector at 5+ minutes and read "(stale)" three of every five minutes on a healthy server. Cards and list_servers rows now carry stale_threshold_minutes so the flag stays recomputable from the payload; Lite's tooltip quotes the stamped cutoff instead of the constant. The null cpu_percent/memory_mb on those cards is a separate structural gap (the fleet reader only queries SQL Server metric views), filed as #3267 - the metric reads are unbounded latest-row reads, so there was no too-tight window to widen here. Closes #3236 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtjWJJ3NnSkDSrPBsGywoZ
…dence Make the server stale band honor each collector's own cadence (#3236)
The wave's fix PRs deliberately carried no CHANGELOG edits (parallel-agent hot-spot protocol); each agent reported its entry and this commit lands them together, byte-verified against origin/dev. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dqza1QNL4YCFS1MpzezERn
…lice Add the #3236 CHANGELOG entry (wave consolidation follow-up)
…e fix wave Takes dev's CHANGELOG wholesale and re-runs the byte-preserving split: the wave's 14 entries land under [3.7.0] - 2026-09-10 (278 entries), [3.6.0] unchanged at 111, entry multiset verified unchanged, census CRLF-only. README auto-merged (tool counts coexist with the wave's PostgreSQL rewrites). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dqza1QNL4YCFS1MpzezERn
Reverts PR #3268 and its CHANGELOG entry (#3269). Field review against the pgmon store showed the change is a net regression: EffectiveStaleThreshold takes the max over a server's collectors, so any enabled slow-cadence collector (these PG targets carry 9 hourly + 4 daily) pushes the per-server stale cutoff to tens of hours, while ClassifyFreshness checks the flat 30-minute Offline band first. That leaves the amber Stale band unreachable on every real target: a server reads Fresh until 30 minutes fully dark, then jumps to Offline, so a multi-minute collection gap now shows green. The original #3236 signature was a true positive (a real coverage collapse during the nightly restart), not the false positive the fix assumed, because last_collection is MAX over all collectors and the ~70s collectors keep it current on a healthy server. Reopening #3236 to re-diagnose with fleet measurement in the loop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dqza1QNL4YCFS1MpzezERn
…-cadence Revert cadence-aware fleet staleness (#3268): it deletes the amber band fleet-wide
get_fleet_overview / /api/fleet and the viewer's Overview cards read CPU only from v_cpu_utilization_stats, which the SQL Server ring-buffer collector writes and a PostgreSQL target never has a row in. Instance CPU for those targets lands in collect.pg_cpu_utilization from the Performance Insights API (#2719) and nothing on the fleet path looked at it, so every PostgreSQL card read total_cpu_percent null / cpu_severity Unknown permanently and could only rank in the worst-first list by collector state. One bounded cross-server read over collect.pg_cpu_utilization feeds total_cpu_percent for those cards, through the same CpuSeverity ladder. The per-process split stays null: Performance Insights publishes the host total and no breakdown, so cpu_source names which collector answered instead of leaving a consumer to infer provenance from which fields are null. Memory, buffer pool and the threads block stay null on a PostgreSQL card. No PostgreSQL equivalent is collected and none is invented here.
The shape pins assert the SQL's text and the reduction's arithmetic; neither can see whether the query runs. This exercises the read against a real store: the bare pg_cpu_utilization name resolving through search_path, the naive-UTC bind against a naive timestamp column, and DISTINCT ON picking the row the reasoning says it picks. Four servers share one sample so all four cpu_source arms are observable in one fleet call, with a NULL-valued newest sample and an out-of-bound row in a different band as the two traps a read without the IS NOT NULL filter or without a working freshness bound falls into.
The PerformanceInsights arm's doc and the get_fleet_overview description both read "an Aurora/RDS PostgreSQL target", contradicting NoSourceForEngine's own doc two arms down. PgCpuUtilizationCollector.AppliesTo gates the ingest on IsAurora, so a plain RDS-for-PostgreSQL target lands on NoSourceForEngine and never on PerformanceInsights in this build. The tool description is read by MCP clients deciding how to interpret cpu_source, so it now names that case explicitly rather than leaving two readings available.
MemorySeverity(bool), BlockingSeverity(int, double) and DeadlockSeverity(int) took non-nullable parameters, so a zero could not mean "absent". A PostgreSQL target has no row in v_memory_grant_stats, v_blocked_process_reports / v_dmv_blocking_snapshots or v_deadlocks, so its card was handed zeros and all three answered Healthy — a green dot claiming health about a metric nothing read, and worse than the null beside it. All three now take a nullable reading and return Unknown for absence, the way CpuSeverity and ThreadsSeverity already did. One shared decision (ServerMetricSources.DmvSourced) says when a reading is a measurement, so the fleet card and the viewer's Overview card cannot disagree about the same server. The published counts and #3017's deadlock_source are deliberately unchanged: the fleet total is summed from those zeros and the coverage block is what explains it. Only the band stops claiming health — which makes the dot agree with the deadlock_source beside it instead of contradicting it. Adding an Unknown cannot move a band or a rank, because both reducers already ignore it: OverallMetricSeverity escalates only on Warning and Critical, and FleetHealthScore's magnitude counts only those two. That is pinned, so an "improvement" that made Unknown escalate fails a test instead of silently reordering the fleet.
#3271 merged with its entry text in the PR body, per the convention that a lane PR does not touch CHANGELOG.md. Without this splice its two changes ship in the next release undocumented. The second entry cites #3271 rather than #3272: the false-Healthy defect was filed as #3272 and folded into the same PR, and #3272 has since been rescoped to the open question of whether pg_blocking should feed the card's counts, so its body no longer describes this change.
Record the two CHANGELOG entries for #3271
…-CPU work, re-split changelog Rebuilds the [3.7.0]/[3.6.0] split from dev's corrected CHANGELOG (carries #3271); byte-preserving, CRLF-only, entry multiset unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WejwpgWF5Xfm3fAmoEbFz4
…shipped release The guard derived the expected fixture from the top changelog heading, which at a cut is the version being cut (3.7.0) - a fixture of which is already at the ladder top and climbs nothing, failing the climb theory's applied>0 assertion. It now targets the newest SHIPPED release (excluding the in-development Directory.Build.props version), i.e. 3.6.0, the store users upgrade from. migration-ladder-v3.6.0.sql generated from the v3.6.0 tag (103 rungs, top V104) so the current V114 ladder replays 10 real rungs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WejwpgWF5Xfm3fAmoEbFz4
Prepare 3.7.0: version bump, changelog cut (with the missing 3.6.0 section), README tool counts
Release v3.7.0
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )