collection health: distinguish STOPPED (gone dark) from FAILING (still erroring)

[erikdarlingdata]

What

get_collection_health and the fleet overview band a collector FAILING purely from hours-since-last-success. A collector whose AppliesTo gate flips off for a target (agent_status / running_jobs on an RDS SQL Server instance, confirmed today) simply stops being invoked — no new success, no new error, nothing at all. Its old success ages past the FAILING cutoff and it reads exactly like a genuinely broken collector, which is real noise: every server on the use2 fleet shows "2 collectors failing" from this alone, and it's exactly the kind of noise that will mask a real problem during UAT this week.

Fix

New CollectorHealthClassifier.Stopped band, keyed on hours since the newest run of ANY status (last_run_time, already selected in every health query) rather than hours since last success:

• A collector that keeps being invoked and keeps failing has a small hoursSinceLastRun (the failure itself is a run) → still correctly FAILING.
• A collector that has attempted nothing at all for that long → STOPPED.

hoursSinceLastRun can never exceed hoursSinceLastSuccess (every success is a run), so this strictly recategorizes a subset of what used to read FAILING — it never suppresses a real failure. STOPPED does not count toward a server's failing-collector total, matching how NO_PERMISSIONS already behaves (confirmed: ReadFailingCollectorCountsAsync filters on the literal "FAILING" string).

Wired through all four HealthStatus call sites (Lite, Darling service, Darling viewer, Darling fleet rollup) plus the fleet-level SQL, which did not previously select last_run_time at all — only the per-server queries did. HoursSinceLastRun falls back to HoursSinceLastSuccess when unset, so a row can never read more dormant than a known success already implies (matters for hand-built test fixtures; real SQL always sets it once TotalRuns > 0).

What did NOT need a fix (investigated and confirmed correct)

The existing PERMISSIONS/NO_PERMISSIONS design (CollectorRuntimePrecondition) already deliberately buckets "a denied grant, a missing source object, and a platform limitation the collector can't do anything about" under one status, with the distinguishing detail carried in the quoted server error text rather than a second status word — a documented, considered design decision. The fleet rollup already excludes NO_PERMISSIONS from its failing count. No change made there.

Tests

Decision-table cases added to both CollectorHealthClassifierTests.cs copies proving STOPPED vs FAILING given identical hoursSinceLastSuccess but different hoursSinceLastRun; three existing cases reclassified from FAILING→STOPPED with explanatory comments (100%-success collectors whose only clock is last-success are, by definition, the STOPPED shape once no run of any kind exists since). Object-based fact tests in ViewerDailyHealthTests.cs updated the same way.

🤖 Generated with Claude Code

[claude]

Review

Went through the classifier change, all four HealthStatus call sites (Lite, Darling service, Darling viewer, Darling fleet rollup), both test suites, and the SQL Server T-SQL style rules from CONTRIBUTING.md (not applicable here — this PR touches only C#/Npgsql code and a CHANGELOG entry, no .sql collector files).

Correctness — looks solid. The STOPPED band is checked before FAILING/STALE and reuses FailingThresholdHours. Since last_run_time = MAX(collection_time) over all statuses while last_success_time is MAX over a status subset, hoursSinceLastRun <= hoursSinceLastSuccess always holds at the SQL layer, which is exactly the invariant the code comments claim — so this is a strict reclassification of a subset of what used to read FAILING, never a suppression, and STALE/WARNING/HEALTHY paths are provably untouched. Traced the full decision table (recent-failure-still-FAILING vs. gone-dark-STOPPED, the 999-sentinel/NO_PERMISSIONS precedence cases) against ServerHealthBands.cs:664-726 and it all checks out.

Parity — no drift found. All 4 production call sites (DarlingDataReader.cs, ViewerDataService.CollectionHealth.cs, LocalDataService.CollectionHealth.cs, DarlingFleetReader.cs) plus both CollectorHealthClassifierTests.cs copies were updated consistently, with near-identical doc comments and identical get_collection_health tool-description text added to both Lite/Mcp/McpHealthTools.cs and Darling/.../DarlingMcpDataTools.cs. Confirmed ReadFailingCollectorCountsAsync and the viewer's GetCollectorHealthCountsAsync/MainWindow.ServerManagement.cs failing-counts all filter on the literal "FAILING" string, so STOPPED is excluded from failing totals automatically, matching the existing NO_PERMISSIONS behavior as claimed — no extra wiring needed there. HealthStatus renders as plain text in the viewer grid (confirmed via the XAML comment at ViewerServerTab.xaml:3435), so no color/brush switch needed a new case either.

Two stale docs (not part of the diff, so I couldn't leave inline comments — GitHub only accepts review comments on lines within the PR's diff):

• docs/how-collection-works.md:107 enumerates the full classifier band set (NEVER_RUN, NO_PERMISSIONS, FAILING, STALE, WARNING, HEALTHY) and says "one classifier is shared... so the three can't drift" — worth adding STOPPED there.
• Darling/README.md:909 lists the Health Summary band set in the same enumerated form and is missing STOPPED too.

Neither is a functional issue, just doc completeness given this PR is explicitly about the shared classifier's contract.

No security, SQL injection, or performance concerns — this is pure in-memory banding logic over already-fetched aggregate rows, no new query shapes beyond the one added last_run_time column in the fleet rollup SQL (which mirrors the per-server queries that already selected it).

[claude]

Review

Solid, well-contained change. Traced the new STOPPED band through all three HealthStatus call sites (Lite's LocalDataService, Darling's ViewerDataService, Darling's service-side DarlingDataReader) plus the two fleet-level SQL aggregates (DarlingFleetReader.FleetCollectionHealthSql and the viewer's own copy) and confirmed:

• Precedence is correct: NEVER_RUN → NO_PERMISSIONS → on-load exemption → STOPPED → FAILING → STALE → WARNING → HEALTHY, matching the updated doc comment in ServerHealthBands.cs.
• Column wiring is correct: the new last_run_time column appended to DarlingFleetReader.FleetCollectionHealthSql lines up ordinally (index 7) with the LastRunTime = reader.IsDBNull(7) ? ... : reader.GetDateTime(7) read below it. The viewer's own FleetCollectionHealthSql/CollectionHealthSql already selected last_run_time before this PR, so no gap there.
• STOPPED is correctly excluded from failing counts everywhere: ReadFailingCollectorCountsAsync, GetCollectorHealthCountsAsync, and MainWindow.ServerManagement.cs all filter on the literal "FAILING" string, so STOPPED falls out naturally without needing a code change at those sites — consistent with how NO_PERMISSIONS already behaved.
• Lite/Darling parity holds: Lite.Tests/CollectorHealthClassifierTests.cs and Darling/Darling.Tests/CollectorHealthClassifierTests.cs diff down to only namespace/header/comment wording — the actual InlineData decision table is byte-identical. The get_collection_health MCP tool description was updated identically in Lite/Mcp/McpHealthTools.cs and Darling/.../DarlingMcpDataTools.cs.
• The fallback in HoursSinceLastRun (LastRunTime.HasValue ? computed : HoursSinceLastSuccess) is sound: real SQL always populates last_run_time via MAX(collection_time) in the same aggregate query whenever TotalRuns > 0, so the fallback only ever engages for hand-built test fixtures, exactly as the PR description claims — verified by grep across all query sites.
• No SQL injection, secrets, or missing-index concerns — this is a pure C# classifier change plus one additive, parameterized column in an existing aggregate query.

One very minor nit, not blocking: ViewerDataService.Overview.cs line 329's doc comment on GetCollectorHealthCountsAsync lists the bands that count as "neither" healthy nor failing (STALE / WARNING / NO_PERMISSIONS / NEVER_RUN) and wasn't updated to mention the new STOPPED band. The code itself is unaffected (it only matches the literal "FAILING" string), so this is a doc-only omission — couldn't leave it inline since that file has no lines changed in this PR's diff.

[claude]

Reviewed the diff (excluding CHANGELOG.md content, which is fine on its own — see line-ending note below).

Correctness — looks solid. The core fix is a clean, minimal addition: Classify now takes hoursSinceLastRun alongside hoursSinceLastSuccess, and the new STOPPED branch sits between the on-load exemption and FAILING — checked before FAILING on hoursSinceLastRun rather than hoursSinceLastSuccess, so a collector that's still being invoked and erroring every cycle (small hoursSinceLastRun) correctly still falls through to FAILING. The invariant the PR description leans on (hoursSinceLastRun can never exceed hoursSinceLastSuccess) does hold in the SQL: last_run_time is MAX(collection_time) over all statuses while last_success_time is the same MAX filtered to SUCCESS/SKIPPED, so STOPPED really is a strict recategorization of what used to read FAILING, never a suppression.

Traced HoursSinceLastRun's fallback (LastRunTime ?? HoursSinceLastSuccess, not the bare 999 sentinel) through all three CollectorHealthRow/CollectorHealth definitions (Lite, Darling viewer, Darling service) — consistent everywhere, and it's what makes the reclassified hand-built test fixtures (which never set LastRunTime) come out STOPPED instead of something stranger.

Failing-count exclusion confirmed correct without any code change, as the PR claims: every consumer (DarlingFleetReader.ReadFailingCollectorCountsAsync, ViewerDataService.Overview.cs, MainWindow.ServerManagement.cs) filters on the literal string "FAILING", so STOPPED rows are naturally excluded the same way NO_PERMISSIONS already is — verified this is not accidental, since CollectorCounts only tracks Healthy/Failing buckets.

Fleet SQL column ordinal double-checked — DarlingFleetReader.FleetCollectionHealthSql's new last_run_time lands at SELECT position 7, and reader.IsDBNull(7) / reader.GetDateTime(7) matches. The four call sites (Lite LocalDataService, Darling DarlingDataReader, Darling ViewerDataService, and the newly-fixed Darling fleet SQL) all stay in parity — same property, same fallback, same threshold reuse (FailingThresholdHours, not a new constant). No Lite/Darling drift found.

UI layer: confirmed StatusToBrushConverter doesn't need a STOPPED case — the Collection Health grid's HealthStatus column is plain text in both apps (per the existing ViewerServerTab.xaml comment), so this doesn't touch color-mapping at all.

Tests: the decision-table and fixture reclassifications read as intended — e.g. the still_failing_collector vs stopped_collector pair in ViewerDailyHealthLivePostgresTests is a good live-Postgres proof that a recent error keeps a collector FAILING even with an old last-success.

One nit: this PR flips CHANGELOG.md's line endings from CRLF to LF for the entire file (base has \r\n throughout, this branch has bare \n). .gitattributes sets the repo default * text=auto eol=crlf with no .md override, so this looks unintentional — it's why the diff shows ~3057/3055 changed lines instead of just the one new entry, and it'll conflict with any other PR touching CHANGELOG.md concurrently. Worth re-saving with CRLF before merge (GitHub blocked an inline comment on this file since the diff is too large to anchor to).

No missing-index-DMV suggestions to make here — no T-SQL changed in this PR.