Release 3.4.0 - #2088
Merged
Merged
Conversation
# Conflicts: # CHANGELOG.md
… path Closes #1940 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
From discussion #1936: an offline monitor rendered a continuous line through the outage. TimeSeriesGaps derives each series' cadence (median positive inter-point spacing, robust against the very gaps being detected) and injects a NaN break mid-gap past 3x that spacing - ScottPlot 5 renders NaN as a line break, endpoints keep their markers. Applied via the Add.TimeSeries extension at all 104 time-series sites across both apps; synthetic zero-lines and placeholders (audited one by one) keep plain Add.Scatter because a cadence is a property of collection timestamps. Closes #1944 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…trip The Settings window showed the PagerDuty boxes and the test button worked, but SeedNotificationControlsFrom never seeded them from the store and BuildNotificationRowFromControls never wrote them - so Save built the row with empty defaults (wiping any stored key) and reopening the window showed blanks. Wired both, following the sibling channels' exact shape: enablement derives from a non-empty key at seed time, and a disabled channel writes empty fields so the service treats it as off. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # CHANGELOG.md
Adding PagerDuty as a first-class webhook notification channel
…through a proxy Closes #1945 pagerduty_proxy threads the sibling channels' exact shape end to end: IAlertSettings, the send (was proxyAddress: null unconditionally), the test buttons, both Settings windows with a proxy row, Lite's plain settings.json pref, and Darling's non-secret column tier - V43 migration, both column lists, upsert $19, the viewer round-trip, the managed-roles and provisioning grants, and a V43 probe sentinel so the viewer's version gate reads the store honestly (the gate/pin tests caught exactly the two spots the StorageVersion-bump discipline says they exist to catch). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… and CSV export ignore gap markers The name-based sweep converted four synthetic spike-baseline arrays (expandedTimes interleaves artificial 0.0001-day offsets around each incident) whose fake deltas would dominate the cadence median and shatter the zero baseline - reverted with the why on each. Two real downstream defects fixed: Enumerable.Min/Max propagate NaN, so one gap marker silently killed a series' entire gradient fill (real values now rank); and both CSV exports wrote the fabricated mid-gap NaN row - exports now carry only collected data. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Break chart lines at collection gaps, cadence-derived per series (#1944)
…e PagerDuty grid, and Lite's proxy box joins the enable toggle The XAML edit dropped the new RowDefinition into the Teams grid, so the PagerDuty test button (Grid.Row 3) clamped onto row 2 and rendered on top of the proxy box. Moved to the grid that uses it. Lite's UpdatePagerDutyControlStates now toggles the proxy box like every sibling channel does. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PagerDuty proxy parity (#1945)
…e verb The #1581 startup classifier bounced the verb to 'Unknown option' before its dispatch could run - full dispatch block, advertised help text, live-tested internals, dead code from the command line. Found on the first real field deploy. A reflection pin now proves IsKnownVerb reaches every Is*Verb classifier on the class, so the allow-list can never silently drift from the dispatch again; watched-red names the orphaned classifier and the mutation sticks at Unknown option. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…unreachable Make --collapse-legacy-slices reachable from the command line (#1912 follow-through)
Field investigation on a 52-replica RDS fleet: both collectors spliced their expensive applies below the TOP, and the optimizer does not defer them - a captured plan showed dm_exec_text_query_plan executing 2,434 times to keep 200 rows (81% of the sweep), with 30-second timeout MISSES on big-cache boxes. Rank on the cheap DMV columns in a derived table first; run text/plan applies and the self-filter against survivors only. Field-validated 6.0x (7.7s -> 1.3s median, 99.2% row parity). The procedure_stats fix is tail insurance - free under the TOP, bounded past it - with the plan handle round-tripped from the varchar the payload already carries. Survivors-only ordering pinned structurally in both definition suites; live-executed verbatim on SQL Server 2025. Closes #1959 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-only ordering on the plan-off paths too Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nder query_stats and procedure_stats rank before they render (#1959)
…1954) A viewer that would not connect gave a generic failure over a log naming neither the config file it chose nor the values it got from it, so "it read the wrong file" and "it read the right file and a value is wrong" produced identical evidence. Startup now logs the resolution before the load is attempted -- which of the four rules won (explicit path, DARLING_CONFIG, beside the viewer, service root), the absolute path it produced, and whether the file exists -- so a missing or unparseable file still says where the viewer looked. Once it loads, a non-secret parse summary follows: host, port, username, database, SSL mode, search path, the derived-vs-verbatim managed flag, and the certificate as written plus the absolute path Npgsql will actually open and whether it is there. A bare Root Certificate resolves against the process WORKING DIRECTORY, so a shortcut launch and a shell launch look in different places; that is now visible instead of inferred. The same block renders in the connection-failure window under a Copy details button, and reaches the log flushed immediately. Redaction is structural: the summary is built from an allowlist of properties read off a parsed NpgsqlConnectionStringBuilder, so no path through it copies the caller's string into its output and no keyword added later can leak by being forgotten. A string that will not parse reports only the exception TYPE (Npgsql parse errors can quote the fragment they choked on, and that fragment can be the credential). A test feeds a live password through and asserts it appears in neither the summary nor the composed block. ResolveConfigPath becomes a projection of the new ResolveConfigLocation rather than a second copy of the rules, pinned by a test, so the diagnostics cannot describe a resolution the load did not perform. ShowMessage now requires a details argument and has exactly one caller, pinned so a new failure branch cannot ship without the context. Also adds the darling-config-env non-parallel collection: the DARLING_CONFIG resolution tests mutate process state, and xUnit runs collections in parallel -- the same hazard ViewerTimeStaticsCollection documents. The layered connection self-test (#1954's third bullet) is not in this change; the issue stays open for it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
) Getting a remote Viewer connected was the hardest part of a real field setup, and every step was work the service could already do: copy JSON out of the docs, hand-merge --print-viewer-connection's terminal output into it, place the certificate somewhere the connection string agrees with, and discover by trial that the VIEWER wants "managed": false while the SERVER runs true. --export-viewer-config writes the viewer machine's whole folder: a complete darling.json (resolved host, port, role credential, verify-full TLS, "managed": false already set, every field documented in comments IN the file), the store's server.crt beside it, and a README.txt carrying the same reference in plain text - including the valid Root Certificate= values, which is what the docs failed to state. Default destination is a viewer-config folder beside darling.json; a positional argument names another. Because that positional inverts the sibling verbs' convention, an explicit config path is --config <path>, and a destination that is really a config file is refused by name before any credential is touched. The exported darling.json holds a live credential, so the verb says so naming the file BEFORE writing it, and hardens the file to SYSTEM + Administrators + the running account + INTERACTIVE. A missing credential or certificate is refused with the exact missing file and the one action that produces it, rather than exporting a folder that cannot connect - verify-full without the PEM is a handoff that fails on the viewer's desk. --print-viewer-connection now emits every STDERR line ahead of the STDOUT payload, the missing-certificate NOTE included; it used to trail the secret. Both verbs were checked with both streams redirected to one handle, which is the only way to see the real order. Resolution is shared: one ResolveViewerHandoffAsync feeds both verbs, so they cannot disagree about host, role, credential or certificate. The load-bearing test feeds the exported JSON to the Viewer's own ViewerSettings parser and requires the exact connection string back - a managed:true export sends that parser hunting for a local bundled PostgreSQL, which is the reporter's bug, now reproduced and caught. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…stops overclaiming #1957: the SECURITY WARNING was honest. After applying the hardened ACL the installer set the service account as owner by handing Set-Acl a freshly constructed FileSecurity carrying nothing but that owner. Set-Acl applies the whole descriptor it is given, so that call also wrote an empty, unprotected DACL - re-enabling inheritance and handing BUILTIN\Users read back on an install under C:\, one statement after the hardening. Confirmed on a scratch layout under C:\ before changing anything: immediately after the owner step the file read protected=False with BUILTIN\Users present, all four inherited ACEs back and every hardened ACE gone. #1818's startup sweep re-hardened everything seconds later at the first service start, which is why the operator's own before/after captures both looked clean and only the installer disagreed. The owner now goes onto the file's CURRENT descriptor, so the hardened DACL rides along and the verified state is the final state. Proven by running the SHIPPING hardening block against the scratch layout: fresh install and a second install over the already-hardened layout both report 3 hardened / 0 warned with protected=True, no BUILTIN\Users, owner = the service SID; and a control where the harden genuinely fails still names the files, which are world-readable on disk at that moment. The per-file verification predicate is untouched - it was correct throughout. The install summary now states that the service re-verifies these ACLs at every start. #1958: the retention summary read "(raw 4 days, hourly CAGGs 90 days; daily CAGGs kept indefinitely)" - a universal claim with three counterexamples in timescaledb_information.jobs, where the docs send an operator to check it. query_store_stats_interval_hourly carries 7 days by design (#1849's dedup tier, sized to outlive raw), its daily twin carries 10, and the nine baseline aggregates keep 35. The line now names every tier that has a policy, with every number interpolated from its constant per #1942. Darling/README.md gains the rollup-tier ladder, which was not documented anywhere user-facing - its Retention section described only the collector horizons, so the three rolled-up raw tables read as 30 days when a 4-day policy is what actually drops them. Both fixes are newly pinned; neither line had a test before. The summary pin derives its expectation from RetentionPolicies itself, so a tier added on a new horizon fails until the summary mentions it. Watched red: the summary pin fails naming IntervalRetentionInterval against the old wording, and the installer pin fails both against the bare descriptor and against a second bare descriptor left after a correct re-read. Also fixes two pre-existing xUnit2013 warnings on dev in ProcedureStatsCollectorDefinitionTests, restoring the zero-warning build. Closes #1957 Closes #1958 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The overlay covers the whole content grid, including the sidebar that holds View Log / Open Log Folder -- so the log was unreachable from the one screen where it is most wanted. The diagnostics block deliberately omits the exception and its inner chain; the log has both. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The JSON and the README record the same act; sampling UtcNow twice can straddle a second boundary and print two different timestamps for it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fn_trace_gettable has no predicate pushdown, so handing it the BASE path with max_files materialized all five 20 MB rollover files every cycle and the StartTime watermark filtered only afterwards. The reporting fleet measured 897 ms whole-set against 178 ms current-file on its apex box. Steady-state cycles now read only the current file and fall back to the unchanged base+max_files form whenever the trace has ROLLED since the previous cycle, because post-watermark events then span two files. The comparison and the read both use ONE captured value of t.path, so the path recorded for the next cycle is provably the file this cycle read; deciding host-side would need a pre-read a rollover could land inside. The decision needs per-server state the collected rows cannot produce -- the cycles that most need it collect nothing -- so collector_state joins both stores (Lite creates it on start, Darling migrates to V44), keyed per server and collector. No stored path takes the fallback. Proven live on SQL Server 2022 and 2025: identical counts and identical CHECKSUM_AGG for a cutoff inside the current file's span at 4.4-4.9x, and 1,161 vs 4,497 events for a cutoff spanning a rollover, where the fallback is what keeps the read lossless. Closes #1962 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The store-schema probe caps at its newest known sentinel, so a V44 store with no rung for collector_state reads as V43 -- and the connect-time gate compares the probe against RequiredStoreSchemaVersion, which means every fully-migrated store would be refused as skewed. The viewer never reads collector_state (service-only state, no view, no query), so this rung is purely about not under-reporting. Also bumps the Lite table-count pin (45 -> 46) and clears the two pre-existing xUnit2013 warnings in the procedure_stats definition tests, so -t:Rebuild is back to zero warnings solution-wide. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Security review of the new verb found the export could overwrite files
it had no business touching. Reproduced against the built binary before
fixing: --export-viewer-config <install dir> replaced the SERVICE's own
390-byte darling.json with the 2813-byte viewer config, exit 0, no
warning - taking every monitored server, every DPAPI encryptedPassword
and the MCP/web tokens with it, none of which exist anywhere else. The
install directory is the obvious place to put a handoff folder, so this
was one keystroke from a legitimate command.
Now refused, before any config load or credential decrypt:
- a destination whose darling.json IS the service's own config
- a destination holding a darling.json this verb did not write, which
is either an operator's file or one pre-created by a local user who
would keep OWNERSHIP of it through the harden (a Windows owner keeps
WRITE_DAC and can undo any DACL applied afterwards)
- a destination that is a junction or symlink, which needs no
privilege to create and redirects the cleartext credential
Re-exporting over this verb's OWN output stays silent - it is the
documented step after a credential or certificate rotation.
The file is now deleted before being rewritten, so this process owns it
rather than inheriting a pre-planted owner; a write that dies part-way
takes the partial secret with it and says so.
The harden is verified, not assumed: every sibling call site pairs
HardenFile with IsReadableByOrdinaryUsers, and this one did not while
its own comment claimed it mirrored them. A secret left readable is now
a WARNING with delete-it advice and a non-zero exit, so automation
cannot read "exported" as "protected".
Argument parsing is strict, in the spirit of #1581: an unrecognized flag
used to become the destination, and a bare --config would have written a
live password to a folder literally named "--config" under the CWD -
C:\Windows\System32 for the elevated prompt the docs prescribe.
Filed #1970 for the viewer-side half the review found: a relative
Root Certificate resolves against the Viewer's working directory, so
that anchor belongs in ViewerSettings, not in this verb.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The doc comment described the verb as it was before the security round. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-config # Conflicts: # CHANGELOG.md
Tiny fixtures compressed LARGER than their text (gzip framing beats a 60-char payload), failing the smaller-than-text assertion. Real plans are KBs; the fixtures now carry ~6 KB of operator-block bulk so the assertion tests the property it names. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add --recompress-plan-dim: convert pre-V54 plan-dim text rows to gzip (#2076)
… volume (#2076) Shipping the conversion without the compaction leaves operators with a file parked at its high-water mark and a df that never moved -- the freed space is real but internal, and "go discover VACUUM FULL" is not a process. When the conversion CONVERGES with zero verify failures the verb now rewrites the dimension to its live content: - Preflighted against free disk on the store's own volume (the sampled compacted-size estimate x1.2 headroom; the reused --backfill-rollups free-space resolver). Refused with numbers when it will not fit or the volume is unmeasurable -- the conversion stays committed either way, and --vacuum-full retries the compaction alone. - Disclosed as the one exclusive-lock step: collections queue for its duration (typically minutes); the service does not stop. - --no-vacuum-full skips it for operators scheduling the lock window separately; --vacuum-full compacts an already-converted store (the fleet store converted by the pre-compaction build is exactly this). - Mid-run arrivals leave pending rows: compaction defers until a run actually converges, and says so. - Flag grammar is a PURE parser (ParseRecompressArgs) that REFUSES unknown --flags instead of treating a typo as a config path. Tests: VacuumFullSql targets exactly the plan dim; the estimate SAMPLES the gzip average (LIMIT) rather than detoasting the whole dimension; the parser grammar pins including the typo refusal; the live test now runs VACUUM FULL after convergence and proves a converted row still decompresses to its original text after the rewrite. Closes #2076 (follow-up) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
--recompress-plan-dim ends with VACUUM FULL so the saving reaches the volume (#2076)
The reference compaction (174 GB dimension, 7.1M plans -> 77 GB) held its exclusive lock for ~46 minutes, during which collection freshness degraded to 15/52 and recovered fully within five minutes of release. 'Typically minutes' was a guess the measurement contradicts; the verb output, dry-run text, and CHANGELOG now carry the measured number and tell operators to scale by their dimension. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Recompress verb: state the measured lock window, not 'typically minutes'
…latest) The darling-linux job skipped everything on release, so a stable release shipped no linux tar.gz and no versioned container tag -- the compose quickstart pointed released users at :nightly. The job is now the Linux publisher on the release event, mirroring nightly.yml's linux job with versioned outputs: tar.gz + SHA256SUMS-linux.txt uploaded to the release, ghcr pushed as :<version> and :latest. PR/push behavior is unchanged (build-proof only, path-filtered); permissions gain contents:write + packages:write for the uploads. Linux binaries remain unsigned by design -- SignPath signs Windows PEs; checksums are the integrity story. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Release event publishes the Linux artifacts (tar.gz + ghcr :version/:latest)
…ub provenance for the tarball SignPath's cosign support is edition-gated, so the container signature comes from Sigstore keyless instead: Fulcio issues a short-lived cert against the job's OIDC identity, the signature lands in ghcr beside the image, Rekor logs it, and no key material exists anywhere to manage. The tarball gets GitHub-native SLSA provenance via attest-build-provenance (gh attestation verify). Nightly is the proving ground: once a signed :nightly verifies from a client machine, the release arm copies proven steps. Permissions: id-token:write (OIDC identity) + attestations:write (provenance storage) at workflow level; neither grants anything else. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Nightly signs the linux artifacts: keyless Cosign image signature + tarball provenance
…arball provenance Ports the nightly's proven steps (verified from a client 2026-08-06: cosign validated claims, Rekor entry, and workflow identity; gh attestation verify confirmed SLSA v1 provenance on the tarball) into the release arm with versioned tags. SignPath stays the Windows Authenticode signer; its cosign/GPG elements are edition-gated, and the Sigstore path needs no keys or subscription. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Release signs the linux artifacts: keyless Cosign image + tarball provenance
Bumps the three shipped csprojs and retitles the CHANGELOG's Unreleased section. Headlines of this release: gzip plan-dimension storage (V54, 14.4x measured on 885 GB of production plans) with the --recompress-plan-dim operator verb (verified conversion + VACUUM FULL compaction, both validated end-to-end on the 52-replica store), store self-metrics (V53), Lite Query Store backfill, persisted finding drill-downs (V52), Azure SQL DB support for the backfill worker, stable releases shipping signed Linux artifacts (Sigstore + SLSA provenance), and the configure-network comma-splice fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Version 3.4.0
…2090) Every self-alert fires with an explicit severity that rode AlertOutcome.Severity into the log line and then died: the channel builders read only Context.SeverityOverride, and self-alerts deliver with Context: null -- so Collection Stopped rendered INFO-blue in Teams while its log line said Critical. The deliverer now folds outcome.Severity into the context once, upstream of every channel (??= so an explicit override from a context builder wins), and the folded context serializes into alert history so replays keep it. Backstop arms in AlertSeverity.ForMetric for the six self-alerts (Critical, matching their fire sites) and Version Store (PVS) (WARNING-amber; #1984 deliberately ships it without a severity tier) -- the replay renderer has no context by design. Plus the tripwire: a test enumerating EVERY fired metric name against the map, because the #1136 fall-through has now shipped five times and nothing forced a new alert's author to visit the map. Part 1 of #2090; the machine-readable category/severity payload facts follow separately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fold fire-site severity into the alert context so channels render it (#2090)
ReviewThis PR promotes Note: Overall: this is an unusually well-documented release — most changes have inline rationale tied to issue numbers and matching pin tests, and the large majority of the new cross-app logic is genuinely shared code, so parity holds by construction in most places. I found one high-confidence correctness bug and a handful of smaller parity/security items, posted as inline comments. Everything else the review agents flagged was low-severity/cosmetic or already correct — summarized below for completeness. Inline comments posted
Also reviewed, no action needed
Given the size of this PR, this was a sampled/parallelized review of the highest-risk areas (T-SQL, security-sensitive new code, and every new cross-app feature) rather than an exhaustive line-by-line pass over all 582 files. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Comment on lines
+113
to
+116
| IF | ||
| ( | ||
| @product_version >= 16 | ||
| OR @engine = 5 |
There was a problem hiding this comment.
High: @replica_group_available gate omits Managed Instance, contradicting the C# logic this comment says it mirrors.
The comment above (lines 101-105) states this is the "Same gate, same reasoning, as QueryStoreCollector.hasReplicaAttribution" and says "Managed Instance (engine 8) keeps the pure version gate." But PerformanceMonitor.Collectors/QueryStoreCollector.cs:594 defines:
bool hasReplicaAttribution = productVersion >= 16 || context.Target.IsAzureSqlDb || context.Target.IsAzureManagedInstance;MI is included unconditionally, not gated on product version — and the surrounding C# comment (lines 552-554) documents live verification that a Managed Instance can report a pre-16 ProductVersion while replica_group_id already exists.
With the gate as written, @replica_group_available stays 0 on exactly that platform, so the aggregation subquery groups without replica_group_id — summing a secondary replica's executions into the primary's, which is the double-counting bug (#1907) this change is meant to fix.
Notably, the @new gate just above (line 84) already uses @engine IN (5, 8) for the same "Azure evergreen" reasoning, which makes this look like a copy/paste omission of 8 rather than intentional. Suggest OR @engine IN (5, 8).
| "auth": "sql", | ||
| "username": "DarlingMonitor", | ||
| "password": "file:/run/secrets/sql_password", | ||
| "trustServerCertificate": true |
There was a problem hiding this comment.
Medium: insecure quickstart defaults.
This new Docker Compose quickstart sample sets trustServerCertificate: true here (disables TLS certificate validation for the connection to the monitored SQL Server) and allowFrom: "0.0.0.0/0" for both web and mcp network blocks below (lines 22, 31), which disables the CIDR allowlist entirely. Combined with listen: 0.0.0.0 and docker-compose.yml publishing 5153/5152 on all host interfaces with no TLS termination anywhere in the compose path, a copy-paste deployment of this sample exposes the dashboard/MCP endpoint to any IP that can reach the host, protected only by a bearer token sent over plaintext HTTP.
Since this is the literal file operators are told to cp and run (per docker-compose.yml:6), consider shipping safer defaults (a real CIDR, trustServerCertificate: false) or at least a comment warning that a TLS-terminating reverse proxy is expected before exposing beyond localhost.
| string contents; | ||
| try | ||
| { | ||
| contents = File.ReadAllText(path); |
There was a problem hiding this comment.
Medium: file: secret references are trusted with no ownership/permission check.
File.ReadAllText(path) reads the referenced secret file unconditionally. This is the primary secret mechanism for the new Linux/container deployment path (Darling/compose/darling.sample.json uses file:/run/secrets/... throughout), but unlike the existing Windows credential path — where DarlingFileSecurity.IsTrustedOwner/IsReadableByOrdinaryUsers exist specifically to reject a credential file that isn't owned by a trusted principal or that's group/world-readable — there's no equivalent check here. Darling/compose/secrets/README.md tells operators to chmod 700/chmod 600 by hand, but nothing in the code enforces or verifies it, so a misconfigured or tampered secrets directory is silently trusted.
| AND collection_time >= $2 | ||
| AND collection_time <= $3 | ||
| AND recommendation_name IS NOT NULL | ||
| ORDER BY collection_time DESC, score DESC |
There was a problem hiding this comment.
Lite/Darling parity drift — two issues in this file:
-
NULL vs
""in MCP JSON output. This reader (lines 86-119, 137, 177) keeps nullable text columns (query_text,recommendation_state,force_last_good_plan_reason, ...) as realnullwhen the source column is NULL. Lite's equivalent (Lite/Services/LocalDataService.PlanCorrection.cs:87,158) coalesces the same NULLs to"". These are common, legitimately-NULL cases — e.g.force_last_good_plan_reasonis NULL whenever desired state equals actual state, the healthy/common path — soget_plan_correctionsreturns a structurally different JSON shape (nullvs"") between the two apps for the same real-world condition. Worth picking one convention and matching it (note Darling's own Viewer-side reader,ViewerDataService.PlanCorrection.cs, already uses the""convention like Lite — this MCP reader is the outlier). -
Missing
ORDER BYtiebreaker. This query sortsORDER BY collection_time DESC, score DESConly, while both apps' grid queries and Lite's MCP tool (Lite/Services/LocalDataService.PlanCorrection.cs:68) add, recommendation_nameas a tiebreaker. When multiple rows sharecollection_timeandscore(common — one collection cycle can produce several rows for a database), Postgres gives no ordering guarantee among ties, so which rows surviveTake(limit)can vary between runs and diverge from what Lite would show for equivalent data.
Comment on lines
+137
to
+140
| /* Attributes of the interval rather than measurements of it — query text, hashes, the forced-plan | ||
| flags. Every slice carries the same value, so ANY_VALUE is correct; DuckDB has it, which is why this | ||
| does not need Postgres' bool_or special case. */ | ||
| return $"ANY_VALUE({column})"; |
There was a problem hiding this comment.
Lite/Darling parity: collection_id combine rule is non-deterministic here, unlike Darling.
This fallback's doc comment claims "every slice carries the same value, so ANY_VALUE is correct" — true for query_text/hashes/forced-plan flags, but not for collection_id, which is generated per physical row (GenerateCollectionId(), called once per row in RemoteCollectorService.DefinitionRunner.cs WriteBatch). The two split-slice rows being collapsed here genuinely have different collection_id values, so this picks an arbitrary survivor.
Darling's equivalent (Darling/PerformanceMonitor.Darling.Storage/QueryStoreSliceRepair.cs:82-86) explicitly special-cases collection_id to min(s.collection_id) for determinism. Low real-world impact today (no known consumer joins on collection_id), but worth aligning Lite to the same MIN(...) special case, or documenting the divergence as intentional.
| { | ||
| if (!IsReadOnly) | ||
| { | ||
| await using var seed = _dataSource.CreateCommand(DatabaseStateSeedSql); |
There was a problem hiding this comment.
Lite/Darling parity: editor never prunes stale auto-baselines here.
This only runs the seed step (DatabaseStateSeedSql). Lite's equivalent (Lite/Services/LocalDataService.DatabaseStates.cs:132-136, via GetDatabaseStateDeviationsAsync) runs both seed and prune ("tidy auto-baselines for databases no longer in the newest snapshot") every time the editor loads. Darling does have the prune SQL (PruneDatabaseStateExpectedSql in DarlingAlertReadAdapter.cs:690-700), but it's only ever invoked from the background alert-check sweep, never from this Viewer path.
Doesn't cause false alerts (orphaned rows never match the "latest snapshot" join used by both the alert read and this editor's own display), but if the "Database state changed" alert is ever disabled, or on a read-only seat, config.database_state_expected will accumulate orphaned rows for dropped/renamed databases indefinitely on Darling while Lite stays tidy. Consider calling the existing prune SQL here too.
The #1816 sweep re-attempted its ACL rewrite on every start for every darling.json.bak-*, and the rewrite needs OWNERSHIP -- which the error message's own icacls remediation does not transfer. An operator who followed the instructions closed the exposure and still got the ERROR line every start about a file that was already secure (the independent READABLE-by-Users CRITICAL check stayed silent, correctly). The sweep now gates on IsReadableByOrdinaryUsers: exposure closed means nothing to do and nothing to report; the CRITICAL check remains the witness for the still-exposed case. Canary-ACE test pins that an already-hardened backup is skipped untouched, not re-hardened. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Skip already-hardened config backups in the startup ACL sweep (#2093)
The onboarding path refused every SQL-auth password off-Windows, which dead-ends compose deployments: the store-authoritative control plane means darling.json edits do not add servers after first seed, so add_servers IS the designed path -- and it demanded DPAPI on a platform that has none. A reference is a pointer, not a secret (the #1804 contract): store it verbatim in the encrypted-password slot and teach ResolvePassword to resolve references there instead of feeding them to DPAPI Unprotect (a reference can never be confused with a base64 blob). Literal passwords still require Windows; the refusal now names the cross-platform alternative. Tests: reference-in-encrypted-slot resolves via env (usedPlaintext false, the no-warn rule); references pass through storage untouched on every platform; literals still round-trip DPAPI on Windows. Closes #2087 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Linux add_servers accepts env:/file: secret references (#2087)
…no-ops (#2097) In the PowerShell ISE, remote sessions, and redirected stdin, ReadLine() returns null immediately and stderr -- where these verbs put their prompts and failure lines -- is not surfaced at all. So --encrypt-password (the FIRST setup step) read as a hung tool, and the --configure-network wizard bailed with a misleading 'No changes made.' Both now write actionable guidance to STDOUT naming the cause and the paths forward (real console, or pipe the value in). The wizard tells EOF apart from an explicit quit: guidance + exit 1 vs the quiet 'No changes made.' + exit 0. Test drives the wizard with an exhausted reader (the exact ISE shape) and pins both behaviors. Closes #2097 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-2097 Interactive verbs explain non-interactive consoles instead of silent no-ops (#2097)
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
2 participants
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.
Promotes dev to main for the 3.4.0 release. Highlights (full detail in CHANGELOG's 3.4.0 section):
--recompress-plan-dimoperator verb (verified conversion + VACUUM FULL compaction), both validated end-to-end on a 52-replica production storeget_store_metrics:version/:latest, keyless Sigstore image signature, SLSA provenance on the tarball--configure-networkcomma-splice fix ([BUG] --configure-network writes invalid JSON (comma after the colon) when a postgres/mcp/web section's last member is a string with no trailing comma #2073), fleet-scope collector-enable fix (Long Queries banner caches the schedule read for the Viewer session; schedule editor shows preset preview indistinguishably from saved state (#2061 follow-ups) #2064), Settings footer ([FEATURE] Settings dialog Save button placement #2063)Smoke: SQL Server 2017/2019/2025 containers (1,300+ clean collections each), Azure SQL DB GP serverless (64 collections), compose quickstart with fresh V54 store, signed-nightly verification of both Linux signatures from a client machine.
🤖 Generated with Claude Code