Repository navigation
Count ABANDONED cycles in collection health and band them (Fixes #2804) - #2867
Conversation
#2803 gave a budget-abandoned cycle its own ABANDONED status so it would stop masquerading as SUCCESS. The write side landed; the read side never caught up. An ABANDONED run incremented total_runs and nothing else -- not success, error, permission denial or yield -- so it grew the failure-rate denominator while contributing nothing to its numerator, and never advanced last_success_time. Total abandonment still reaches FAILING through staleness. The gap was the PARTIAL case: a collector abandoning some cycles while succeeding often enough to stay fresh has an error rate of exactly 0 and reads HEALTHY indefinitely. Measured in production: procedure_stats at 24 abandoned of 1,226 runs and query_stats at 14 of 1,226, both status=HEALTHY errors=0. The count is now first-class (abandoned / abandon_rate_pct on both MCP tools, an Abandoned column on the web grid and both WPF grids) and feeds the shared CollectorHealthClassifier, which bands WARNING above a 0.5% rate. The threshold is positioned by the fleet: across 1,639 (server, collector) pairs and 520,455 runs, only four pairs abandoned anything at all and the per-pair rate distribution is p50 = p75 = p90 = p95 = p99 = 0.000% with a max of 2.157%. 0.5 sits below that observed floor and above the 99th percentile. WARNING rather than a new band: a new string would need learning by four display mappings that fail in opposite directions -- statusToSev defaults to "Unknown", the deprecated Dashboard's converter defaults to Transparent, the same brush it gives HEALTHY. Attribution survives because the abandoned count sits beside the error count in the same row. Read-side only: no new column, no schema rung. Caught while wiring it: the fleet rollup builds its own banding row from FleetCollectionHealthSql and would have kept calling an abandoning collector HEALTHY while every other surface called it WARNING -- and it compiled, because an unset count defaults to 0. That is the #2779/#2784 shape, so the pin asserts over all four banding reads by name rather than over the one that was broken. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MX6HyjsuDCs15qGB2rh4Gy
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| TargetHasUserDatabases = !reader.IsDBNull(13) && Convert.ToInt64(reader.GetValue(13)) != 0, | ||
| /* Appended (#2804). Both reads above compute it, so unlike has_user_databases it is never a | ||
| NULL placeholder on the fleet side — an abandoned cycle is data loss on either surface. */ | ||
| AbandonedCount = reader.IsDBNull(14) ? 0 : Convert.ToInt64(reader.GetValue(14)), |
There was a problem hiding this comment.
Nit: the doc comment on MapHealthRow just above (line 340: "Maps one row of the shared 14-column health projection...") and the one on FleetCollectionHealthSql (line 184: "the same 14-column per-collector aggregate") are now stale — appending abandoned_count here makes both projections 15 columns (ordinals 0–14, as this very mapping shows). DarlingDataReader.CollectionHealthSql's equivalent comment was correctly bumped to "21 columns since #2804" in this same PR; these two just got missed. Worth fixing given how much this codebase relies on exact counts in comments to keep positional-read surfaces honest.
| the surface only inside note_summary's prose, so there was no number to threshold, | ||
| alert or trend on. */ | ||
| abandoned = r.AbandonedCount, | ||
| abandon_rate_pct = Math.Round(r.AbandonRatePercent, 2), |
There was a problem hiding this comment.
The get_collection_health tool's Description(...) blurb (a few lines above) — the contract an MCP/LLM client actually reads to interpret this tool's fields — walks through status, last_note/note_count, target_has_user_databases, sweep_pressure, avg/p95/max_duration_ms, and fanout in real detail, but never mentions the new abandoned / abandon_rate_pct fields added here, or that a WARNING can now come from abandonment rather than errors.
That undercuts this PR's own stated goal ("a WARNING can always be attributed to abandonment rather than to errors") for exactly the consumer — an LLM client calling this tool — that only sees the description text, not these C# comments. Lite's McpHealthTools.cs has the identical description text with the identical gap, so it's at least parity-consistent, but both descriptions should probably gain a sentence explaining abandoned/abandon_rate_pct and that abandonment bands WARNING at a much lower rate (0.5%) than the error rate (20%).
Review summaryTraced the fix end-to-end — this is well-scoped and well-tested. Verified by hand rather than by building (no cross-platform build available here):
Left two nit-level inline comments (doc-comment column counts gone stale in |
Two findings from the review bot, both real. The viewer's MapHealthRow and FleetCollectionHealthSql doc comments still said "14-column"; appending abandoned_count makes both projections 15 (ordinals 0-14). DarlingDataReader's equivalent was bumped in the first commit and these two were missed. The count is load-bearing here -- both projections are read positionally through one shared mapper -- so the mapper's comment now says so explicitly rather than just carrying a number. The get_collection_health Description is the contract an MCP client actually reads to interpret the tool's fields, and it walked through status, last_note, target_has_user_databases, sweep_pressure and fanout in detail while never mentioning the new abandoned / abandon_rate_pct fields or that a WARNING can now come from abandonment rather than errors. That undercut this PR's own attribution goal for the consumer least able to work it out. Added to both SKUs' tool descriptions so they stay in parity. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MX6HyjsuDCs15qGB2rh4Gy
| ## [Unreleased] | ||
|
|
||
| ### Added | ||
| - **A collector losing cycles to the wall-clock budget no longer reports `HEALTHY` with `errors: 0`** ([#2804]) - [#2803] gave a budget-abandoned cycle its own `ABANDONED` status so it would stop masquerading as SUCCESS. The write side landed; the READ side never caught up, so the fact reached every surface as a sentence inside `note_summary` and never as a number. An ABANDONED run incremented `total_runs` and **nothing else** - not success, error, permission denial or yield - so it grew the failure-rate DENOMINATOR while contributing nothing to its numerator, and never advanced `last_success_time`. TOTAL abandonment does still reach FAILING through staleness, because no success lands at all; the gap was the PARTIAL case, where a collector abandons some cycles and succeeds often enough to stay fresh, so staleness never fires, the error rate is exactly 0, and the row reads HEALTHY indefinitely while losing collections. Measured on a production server: `procedure_stats` at **24 abandoned of 1,226 runs** and `query_stats` at 14 of 1,226, both `status=HEALTHY errors=0`. The count is now first-class (`abandoned` / `abandon_rate_pct` on both MCP tools, an **Abandoned** column on the web grid and both WPF grids) and feeds the shared `CollectorHealthClassifier`, which bands **WARNING** above a 0.5% rate. **The threshold came from the fleet, not from taste**: across 1,639 (server, collector) pairs and 520,455 runs in 24h, only FOUR pairs abandoned anything at all - 28 runs, 0.005% fleet-wide - and the per-pair rate distribution is p50 = p75 = p90 = p95 = **p99 = 0.000%** with a max of 2.157%, so the real population is an empty body and a four-point tail spanning 0.60%-2.16%. 0.5 sits strictly below that observed floor and strictly above the 99th percentile, and keeps a lone abandonment quiet in any window under ~400 runs. **WARNING rather than a new band** because a new string would have to be learned by four display mappings that fail in opposite directions - the web's `statusToSev` defaults to "Unknown" while the deprecated Dashboard's brush converter defaults to `Transparent`, the same brush it gives HEALTHY - and attribution survives anyway, since the abandoned count now sits beside the error count in the same row. The fix is **read-side only**: no new column, no schema rung. Caught while wiring it: the fleet rollup builds its own banding row from `FleetCollectionHealthSql` and would have kept calling an abandoning collector HEALTHY while every other surface called it WARNING - and it **compiled**, because an unset count defaults to 0. That is the [#2779]/[#2784] shape, so the pin asserts over all FOUR banding reads by name rather than over the one that was broken. |
There was a problem hiding this comment.
Nit: this line uses [#2803] as a reference-style link, but [#2803]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2803 is never defined anywhere in this file (unlike [#2804], [#2779], [#2784], which are all defined further down). As written, [#2803] will render as literal bracketed text instead of a link.
| /// <summary>Maps one row of the shared 14-column health projection (per-server or fleet) to a <see cref="CollectorHealthRow"/>.</summary> | ||
| /// <summary>Maps one row of the shared 15-column health projection (per-server or fleet, ordinals 0-14) to a | ||
| /// <see cref="CollectorHealthRow"/>. The count is load-bearing: both projections are read POSITIONALLY through | ||
| /// this one mapper, so it must match them exactly (15 since #2804 appended abandoned_count at ordinal 14).</summary> |
There was a problem hiding this comment.
Minor formatting slip: the 2nd/3rd lines of this doc comment lost their leading indentation (/// <see cref=... and /// this one mapper... start at column 0 instead of being indented under MapHealthRow). Doesn't affect behavior, just doc-comment consistency with the rest of the file.
|
Reviewed the diff end-to-end (SQL ordinals, classifier precedence, Lite/Darling parity, MCP tool descriptions, XAML/web wiring). This is a well-verified change — the read-side plumbing checks out:
Left two minor inline nits: a dangling |
Two more nits from the review bot, both real. The new CHANGELOG entry cites [#2803] reference-style but that ref was never defined in the file, so it would have rendered as literal bracketed text instead of a link. Every other ref the entry uses (#2804, #2779, #2784) was defined; this one was missed. The MapHealthRow doc comment I rewrote in the previous commit lost the leading indentation on its continuation lines, which started at column 0 instead of lining up with the rest of the file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MX6HyjsuDCs15qGB2rh4Gy
ReviewWent through the full diff against CONTRIBUTING.md's parity and style rules. This is a clean, read-side-only change with no schema bump, and it's unusually well-verified. Parity (Lite/Darling): Checked all four banding reads plus the two MCP tools and both WPF/web grids.
Correctness: Traced the Nit (non-blocking): In both No security, injection, or SQL-style issues found — all new SQL is parameterized, uses |
1 participant
Fixes #2804.
The defect
#2803 gave a budget-abandoned cycle its own
ABANDONEDstatus so it would stop masquerading as SUCCESS. The write side landed; the read side never caught up, so the fact reached every surface as a sentence insidenote_summaryand never as a number.An
ABANDONEDrun incrementedtotal_runsand nothing else — not success, error, permission denial or yield. So it grew the failure-rate DENOMINATOR while contributing nothing to its numerator, and never advancedlast_success_time.TOTAL abandonment does still reach FAILING through staleness, because no success lands at all. The gap is the PARTIAL case: a collector abandoning some cycles while succeeding often enough to stay fresh never ages into STALE or FAILING, has an error rate of exactly 0, and reads HEALTHY indefinitely while losing collections.
Measured on a production server, 3h window:
The change
abandonedandabandon_rate_pcton both MCP tools, an Abandoned column on the web grid and both WPF grids, and the count feeds the sharedCollectorHealthClassifier, which bands WARNING above a 0.5% rate.Read-side only — no new
collection_logcolumn, no schema version bump.Why 0.5%, from the fleet rather than from taste
Abandonment is not a continuous rate phenomenon. Over 24h across 1,639 (server, collector) pairs and 520,455 runs, only four pairs abandoned anything at all — 28 runs, 0.005% fleet-wide — and the per-pair rate distribution is:
So the real population is an empty body and a four-point tail spanning 0.60%–2.16%. 0.5 sits strictly below that observed floor (catches every genuinely-degraded collector on the fleet) and strictly above the 99th percentile (fires on nobody who is not abandoning). It also keeps a lone abandonment quiet in any window under ~400 runs — the "one in a thousand is noise, twenty-four is a finding" line.
Deliberately far below
WarningFailureRatePercent(20): an error may be transient and is retried, where the 120 s budget is itself generous (#2673 chose it after measuring a 176 s tail), so reaching it means exceeding a bound already set well above normal.Why WARNING rather than a new band
WARNING is already the rate-based "degrading but still running" verdict, which is what partial abandonment is — and a guard firing is not an ERROR. A new band string would have to be learned by four independent display mappings, and the two that do not switch on it fail in opposite directions: the web's
statusToSevdefaults to"Unknown", while the deprecated Dashboard's brush converter defaults toTransparent— the same brush it gives HEALTHY.Attribution is not lost by sharing the band, because the abandoned count now sits in the same row as the error count.
The drift this caught
The fleet rollup builds its own
CollectorHealthfromFleetCollectionHealthSqland would have kept calling a partially-abandoning collector HEALTHY while every other surface called it WARNING — and it compiled, because an unset count defaults to 0. That is the #2779/#2784 shape: one surface fixed, its sibling quietly left behind.So the pin is an invariant over all four banding reads by name (
DarlingDataReader.CollectionHealthSql,DarlingFleetReader.FleetCollectionHealthSql,ViewerDataService.CollectionHealthSql,ViewerDataService.FleetCollectionHealthSql) rather than over the one that was broken.Verification
The Windows suites cannot run on macOS, so both halves were verified against the real artifacts:
Decision table, red-first. A throwaway
net10.0harness ran 15 cases against the real builtCollectorHealthClassifier. All 15 pass. With the threshold disabled, exactly the four abandonment cases go red and reportHEALTHY— reproducing the bug:SQL, against the real store.
CollectionHealthSqlwas dumped from the built assembly (not retyped),PREPAREd verbatim and executed read-only against a production store. It returns 21 columns, andabandoned_countmatches an independentGROUP BYcross-check exactly:At 2.15% and 0.98% both now band WARNING.
Invariant pin, red-first. Removing the fleet reader's
abandoned_countmakesAssert.Contains("AS abandoned_count", ...)fail; restoring it passes.Notes
abandoned_countis appended, never inserted — ordinal 20 on both.Darling.TestsandLite.Testsdiffer only in header and namespace, so the two SKUs cannot drift.CHANGELOG.mdstaged as 2 insertions / 0 deletions with a plaingit add, so Renormalize the five CRLF blobs to match .gitattributes (Fixes #2857) #2858's renormalization is holding.🤖 Generated with Claude Code
https://claude.ai/code/session_01MX6HyjsuDCs15qGB2rh4Gy