Fleet overview: bound the last-collection scan and fix its null-fallback (false Offline)

[erikdarlingdata]

What

Two confirmed bugs behind a live false-positive on the use1 monitoring host today: get_fleet_overview repeatedly flagged a rotating set of actively-collecting servers (multi-20, multi-03, epsilon-01, dummy-01, multi-31, multi-53 across two separate checks) as "Offline - no recent collection" while their real collection_log rows were 22-100 seconds old.

Bug 1 — unbounded full-history scan

DarlingFleetReader.FleetLastCollectionSql was a bare GROUP BY server_id over the whole collection_log table — no WHERE clause at all. That's a full scan of every row ever collected (2.2M rows / 89 MB on the use1 store, growing forever) just to find each server's most recent timestamp, on every single get_fleet_overview call. Same "materialize a bound, don't scan the whole history" mistake fixed elsewhere today (pg_statement_stats #2691, pg_wait_stats #2695), and inconsistent with this file's own documented design ("a fixed ~9 round-trips regardless of fleet size" via bounded/windowed aggregates) and with its sibling FleetCollectionHealthSql, which already carries WHERE collection_time >= $1.

Fix: added the same 48-hour bound so TimescaleDB can chunk-exclude old data. Wide enough that a server genuinely offline for hours still reports its true last-seen time correctly (not zero rows).

Bug 2 — the actual misclassification mechanism (confirmed)

ReadLastCollectionAsync returns Dictionary<int, DateTime> — a non-nullable value type. At the call site, lastCollection.TryGetValue(server.ServerId, out var lastColl) left lastColl as default(DateTime) (0001-01-01) on any dictionary miss. default(DateTime) is not null, so ClassifyFreshness never reached its NeverCollected branch — it fell straight to the age-vs-OfflineThreshold check with an age of ~2000 years, which always bands Offline.

Fix: the call site now propagates a genuine miss as null, correctly reaching ClassifyFreshness's NeverCollected branch → AwaitingFirstCollection=true, IsOnline=null, band=Warning (a bootstrap state) — never a fake-ancient Offline.

Honesty note

I could not catch bug 1's query in the act being slow — an ad-hoc timing sample ran in 840 ms against the live store, not obviously pathological, and no aggressive command timeout is configured for this reader. So I can't prove bug 1 was THE mechanism that dropped rows from the dictionary on the day in question. What I can prove: bug 2 is real and would produce exactly the observed symptom regardless of why a row goes missing, and bug 1 is a genuine, independently-justified architectural fix that makes misses far less likely. Shipping both together closes the loop either way.

Tests

• FleetLastCollectionSql_IsWindowed_NotAFullHistoryScan — SQL-shape pin (mirrors the existing pin style for FleetCollectionHealthSql).
• GetFleetOverview_ServerWithNoCollectionHistory_ReadsAwaitingFirstCollection_NotOffline — live-Postgres regression test: a registered server with zero collection_log rows must band Warning/AwaitingFirstCollection, never Offline.

🤖 Generated with Claude Code

[claude]

Correctness: the 48-hour bound reintroduces a false negative for real outages, in the opposite direction from the bug this PR fixes.

Before this PR, MAX(collection_time) scanned the whole table, so a server that had been dead for a week still had some row in the result — its true (very old) last_collection_time — and correctly banded Offline (ClassifyFreshness → age > OfflineThreshold → FleetHealthBand.Offline, rank 4000).

After this PR, a server whose newest row is older than 48 hours falls completely out of FleetLastCollectionSql's result set (line 213: WHERE collection_time >= $1, bound to now.AddHours(-48) at ReadLastCollectionAsync, line 760). lastCollection.TryGetValue then misses, lastColl becomes null (by this PR's own fix), ClassifyFreshness returns NeverCollected, and ClassifyBand maps that to Warning (rank 2000) with the "Awaiting first collection" label — not Offline.

So a server down for >48 hours now silently downgrades from the worst-first band (Offline, red, rank 4000) to a mid-severity, benign-sounding bootstrap state (Warning/"Awaiting first collection", amber, rank 2000), and drops in the worst-first ranking below genuinely-Critical servers. That's a real outage being masked as "not yet reached," which is a worse failure mode for a monitoring tool than the false-Offline this PR is fixing — a server dead for a week reads as if the service just hasn't gotten to it yet.

The comment above (lines 206-209) reasons about "a server genuinely offline for HOURS" but the 48h window doesn't cover multi-day outages, and neither this PR's tests nor its description address that case — the new live test (GetFleetOverview_ServerWithNoCollectionHistory_...) only covers "zero rows ever," not "had rows, then went silent for days."

Worth considering: keep an unbounded (or much wider, e.g. no upper cutoff) fallback specifically for the "genuinely never seen in the window" case — e.g., a second cheap query only for server_ids missing from the bounded read, or just don't let a real Offline server disappear from the aggregate at all (index-only MAX per server_id can stay cheap without a time bound, since it's not the full-scan GROUP BY this PR is rightly killing).

[claude]

Minor: "matching the sibling FleetCollectionHealthSql's WHERE bound" isn't accurate — FleetCollectionHealthSql is bound to 7 days (AddDays(-7), DarlingFleetReader.cs line 780), not 48 hours. The two queries now use different windows for what both feed into a server's collection/health status, which is worth calling out explicitly rather than implying they match — see the inline comment on FleetLastCollectionSql for why the shorter 48h window on this specific query is the more consequential issue.

[claude]

Reviewed. Both stated bugs (unbounded full-history scan, default(DateTime)-not-null miss handling) are real and the fixes for them are correct as far as the described symptom goes — nice catch tracing the live false-Offline down to the TryGetValue fallthrough.

One correctness concern worth resolving before merge (left inline on DarlingFleetReader.cs): bounding FleetLastCollectionSql to a 48-hour window fixes the full-scan problem but reintroduces a false negative in the opposite direction — a server that's been genuinely down for more than 48 hours now falls out of the query result entirely, and (via this PR's own null-propagation fix) gets classified NeverCollected/Warning/"Awaiting first collection" instead of Offline. That's a real multi-day outage silently downgraded from the worst-first band (rank 4000) to a benign-sounding bootstrap state (rank 2000), which seems like a worse failure mode for a monitoring tool than the bug being fixed. Neither the new tests nor the PR description cover a "had history, then went dark for days" scenario — only "zero rows ever."

Also flagged a small factual mismatch in the CHANGELOG entry: it claims the 48h bound matches sibling FleetCollectionHealthSql's WHERE bound, but that query is windowed to 7 days, not 48 hours.

Lite/Darling parity: not applicable here — Lite has no server-side fleet-overview aggregate equivalent to get_fleet_overview (its FleetView is a local sidebar projection over ServerManager.GetAllServers(), not a cross-server DB query), so there's no counterpart that needs the same fix.

No security or T-SQL style issues — this is Darling/C#-only, parameterized queries throughout, no secrets or injection surface touched.