tempdb used% can no longer exceed 100 — total size now comes from the same snapshot as usage (#2169)

[erikdarlingdata]

Root cause

The FinOps grid computes UsedPct = used_size_mb * 100 / total_size_mb. Those two numbers came from different places:

• used_size_mb — FILEPROPERTY(df.name, 'SpaceUsed'), read inside each database (sys.database_files).
• total_size_mb — sys.master_files.size, which records the size set at configuration time and does not follow autogrowth for tempdb.

So a tempdb that has grown gets its current usage divided by its startup size, and the percentage runs past 100. Nothing is wrong with either number individually; they just aren't from the same snapshot.

Fix

The per-database probe already visits sys.database_files for SpaceUsed — it now captures df.size in that same round trip (no new query, no extra connection), and the payload takes COALESCE(fs.current_size_mb, mf.size …). Both halves of the ratio come from one snapshot. The COALESCE fallback is deliberate: a database whose probe failed (mid-restore, permissions) still reports a total from master_files rather than dropping out of the grid — worse precision, never a wrong ratio.

Every database benefits, since master_files can lag any autogrowth, but tempdb is where it's guaranteed to.

Scope note

This is the on-prem / RDS / Managed Instance path. The Azure SQL Database path already read both size and SpaceUsed from sys.database_files and never had the skew — worth knowing because the reporter's @@VERSION says Microsoft SQL Azure, which Managed Instance also reports, and MI honors the cross-database reference so it takes the affected path. That reconciles "Azure" in the report with a bug that isn't in the Azure SQL DB query.

Testing

Two pins in DatabaseSizeCollectorDefinitionTests: the on-prem query must declare current_size_mb, capture df.size in the probe insert, and prefer it via COALESCE (with the bare master_files total asserted gone); and the Azure path must keep reading in-database sizes with no master_files reference — pinned so a later "unify the two paths" refactor can't quietly move Azure onto the stale source. Collectors and Lite.Tests build clean.

Fixes #2169.

[claude]

Minor style nit: this entry has a trailing blank line before the next bullet, but every other entry in this ### Fixed list (and in ### Added/### Changed above) runs bullet-to-bullet with no blank line between them. Not functionally significant, just inconsistent with the surrounding convention — worth dropping the blank line for consistency.

[claude]

Review summary

Reviewed the fix for #2169 (tempdb reporting >100% used in FinOps Database Sizes).

Correctness — The root-cause diagnosis checks out: sys.master_files.size is a server-level catalog view that can lag a database's actual current file size (guaranteed-stale for tempdb, since it never persists autogrowth back to the configured startup size), while FILEPROPERTY(..., 'SpaceUsed') is read live, in-database. Mixing the two as numerator/denominator of a percentage can push the ratio past 100.

The fix captures df.size (in-database, via sys.database_files) in the same cross-database probe round trip that already captures SpaceUsed, so both operands of the ratio now come from one snapshot — no new query, no new round trip. COALESCE(fs.current_size_mb, mf.size * 8.0 / 1024.0) correctly falls back to the old (occasionally-stale) source only when the per-database probe failed, and in that case used_size_mb is also NULL from the same failed probe, so the downstream UsedPct calculators (UsedSizeMb.HasValue ? ... : null) never compute a ratio from mismatched snapshots — the fallback degrades precision without ever producing a wrong ratio, as claimed. mf.size is always non-null, so total_size_mb's non-nullable decimal read (reader.GetDecimal(6)) stays safe.

Azure SQL DB path — Untouched and correctly untouched: that path already reads size and SpaceUsed from the same in-database sys.database_files, so it never had the skew. Pinned by the new AzureSqlDb_... test asserting no sys.master_files reference leaks in.

Lite/Darling parity — No drift risk here: DatabaseSizeStatsCollector lives in the shared PerformanceMonitor.Collectors project, and both Lite/Services/LocalDataService.FinOps.cs and the Darling Viewer's ViewerDataService.FinOps.cs compute UsedPct with byte-identical logic downstream, so the fix and the display math both apply to both apps automatically. (The on-prem install/52_collect_database_size_stats.sql script has a similar total/used split, but it's under the already-deprecated Full/Dashboard edition — not part of the live Lite/Darling product — so it's correctly out of scope here.)

Security/Performance — No new dynamic SQL surface (the added df.size reference needs no extra quoting since it isn't a string literal), no new round trips, negligible added query cost.

Tests — The two new tests pin the on-prem query's new column, the COALESCE fallback, and the absence of the old bare-master_files total; plus the Azure path's continued isolation from master_files. Good regression coverage for a "future refactor tries to unify the two paths" scenario, as the test names call out.

One minor style nit left as an inline comment: the new CHANGELOG entry has a trailing blank line that's inconsistent with the rest of the list's bullet-to-bullet formatting. Nothing else to flag — this looks correct and ready to merge.

[claude]

Reviewed the diff (collector + tests + CHANGELOG). Summary: this looks correct and well-scoped, no blocking issues found.

Correctness

• The probe insert and the COALESCE(fs.current_size_mb, mf.size * 8.0 / 1024.0) fallback are sound: when the per-database probe succeeds, used_size_mb and current_size_mb are captured in the same SELECT (same round trip), so both halves of the ratio genuinely come from one snapshot. When the probe fails, the whole cross-db sp_executesql call throws and no #file_space row is inserted at all, so used_size_mb stays NULL too — there's no ratio to compute in that case, so "never a wrong ratio" holds trivially, not just optimistically.
• Verified the new/changed Assert.Contains/DoesNotContain calls in DatabaseSizeCollectorDefinitionTests.cs against the actual generated SQL text in DatabaseSizeStatsCollector.cs line-by-line — they match exactly, including the Environment.NewLine-based negative assertion (safe given .gitattributes normalizes *.cs to CRLF and this codebase already uses that pattern elsewhere, e.g. ForcePlanReplicaScopeTests.cs).
• Units/rounding are consistent: df.size * 8.0 / 1024.0 matches the exact formula already used for mf.size and the Azure path, so no precision drift between the two sources being COALESCE'd.

Lite/Darling parity

• No drift risk here — DatabaseSizeStatsCollector lives in the shared PerformanceMonitor.Collectors project and is consumed verbatim by both Lite/Services/RemoteCollectorService.DatabaseSize.cs and Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs. One fix, both apps get it automatically. Confirmed the existing Darling tests (DarlingPayloadProbeFailureTests.cs) don't assert on the changed SQL text, so nothing there breaks.
• Also checked the legacy T-SQL install artifact (install/52_collect_database_size_stats.sql, the SQL-Agent-driven "Full Edition" path) for the same bug — it already reads total_size_mb from sys.database_files inside the per-database dynamic SQL context (same pattern as the Azure path here), so it was never affected and needs no matching change.
• Confirmed downstream consumers (LocalDataService.FinOps.*, ViewerDataService.FinOps.*, DuckDB/Postgres schema generators) only read the persisted total_size_mb column and don't recompute it, so no other files need touching — the fix is fully contained at the source.

Security/perf

• No new dynamic SQL surface; @db_name is still QUOTENAME'd exactly as before. The fix adds one column to an existing SELECT/INSERT in the same round trip — no new query, no new connection, as the PR description claims.

One very minor nit, not blocking: the CHANGELOG entry writes reported by @CatastropheOps (with @), while other entries in this file use the bare handle (e.g. reported by TrudAX, reported by SalmanRajwani). Inconsistent but cosmetic.

I wasn't able to actually execute dotnet test/dotnet build in this sandboxed review environment, so this is a static read-through rather than a green CI run — worth confirming CI passes, but I don't see anything in the diff that would fail it.