Give a wall-clock-budget-abandoned cycle its own collection_log status

[erikdarlingdata]

Fixes #2801

A collector cycle abandoned by the #2673 whole-server wall-clock budget was recorded as SUCCESS. Every one of the 36 abandonments in the use1 store's entire 17-day retention is SUCCESS with rows_collected = 0.

Why it inherited a success status

The abandonment returns rather than throwing, so it lands on the ordinary success path — whose status is a hardcoded "SUCCESS" literal, once in DarlingWorker and once in Lite's RunCollectorAsync. It carries its message through the Note / telemetry.Note channel, documented since #1837 as being for "a successful-but-empty run worth explaining" and explicitly not status-changing. #2673 reused that channel for a case that is not a successful empty run.

The wrong label was the smaller half. DarlingSelfAlertEvaluator.ReadCollectionSignalsAsync takes both last_success and recent_success from status IN ('SUCCESS', 'SKIPPED') — so a collector abandoning every cycle read as perpetually fresh. And the message landed in last_note / note_count, gated on status = 'SUCCESS'.

The status, and the alternatives rejected

ABANDONED, produced by one shared EnumeratedCollectorDriver.ClassifyReturnedRun so the hosts cannot drift — one hardcoded literal each is exactly how both inherited this.

• SUCCESS — the bug. Nothing stored, no watermark advanced.
• ERROR — would page on a guard doing precisely its job; feeds error counts, health bands and the collection-failure self-alerts.
• YIELDED — documented as the 1s LOCK_TIMEOUT guard and read as evidence of lock contention on the target. Reusing it sends an operator hunting contention that is not there.
• SKIPPED — a healthy no-op that counts as success. This is the opposite: work attempted and paid for that shipped nothing.

Safe to add. Every read buckets by explicit list — IN ('ERROR','PERMISSIONS'), = 'YIELDED', IN ('SUCCESS','SKIPPED') — never by complement, so a new value joins no bucket rather than silently joining the wrong one. collection_log.status carries no CHECK constraint, so no migration rung. And the self-alert's consecutive-failure path is server-scoped across every collector, so one collector abandoning among ~40 healthy ones cannot empty its success window.

What is deliberately NOT changed

• The per-item [BUG] 3.4.0 query_store collector runs 37–100 min on Azure SQL DB (3.3.0 median: 4.8 s) — starves all other collectors #2150 abandonment. It ships rows for the databases it got through, so that cycle really did collect — it correctly stays SUCCESS with its note. Only the whole-cycle Bound each collector's per-server wall-clock footprint (tail is up to 555s on one server) #2673 case, which returns 0 rows before the storage phase, is reclassified. The "partial vs zero" distinction is a property of the code, not a judgement call: the Bound each collector's per-server wall-clock footprint (tail is up to 555s on one server) #2673 site hardcodes 0 rows and its comment notes it skips the storage phase and the state-persistence block.
• A dedicated abandoned_count column in the four collection-health reads. That would touch ~13 sites including four ordinal-based readers, where inserting a column shifts every subsequent ordinal — a silent-corruption risk not worth taking for a display nicety, especially with a fleet roll in flight. Filed as a follow-up rather than bundled. The status change alone already restores the visibility the issue is about: any status <> 'SUCCESS' query now sees these.

Verification

Lite.Tests and Darling.Tests are net10.0-windows and cannot run on macOS, so the pins were also run through a throwaway net10.0 harness against the real built PerformanceMonitor.Collectors assembly. CI is the arbiter for the xUnit run.

Proven red first. Reverting ClassifyReturnedRun to the pre-fix hardcoded literal:

FAIL an abandoned cycle does not classify as SUCCESS
FAIL an abandoned cycle is outside the freshness-success family
PASS an abandoned cycle is not ERROR or PERMISSIONS
PASS an abandoned cycle does not reuse YIELDED
PASS an ordinary returned run still classifies SUCCESS
PASS an ordinary returned run is inside the freshness-success family
PASS the shared note names the budget
2 PIN FAILURE(S)

and with the fix restored, ALL PINS PASS.

The first attempt at that proof was contaminated — the saved copy of the file was written into the harness project directory and compiled into it, shadowing the real assembly (CS0436), so the pins passed for the wrong reason. Re-run with the backup outside the project, which produced the red above.

The pins assert over the freshness-success family rather than against the "SUCCESS" literal, because the defect was a status silently joining a set — a pin naming one member would miss the next status added to it.

The underlying slowness is not new

Worth stating, because the fix makes these newly visible. procedure_stats on the affected server, full retention window:

day	runs	p50	p95	max	over 60s
08-17	32	8.3 s	178.3 s	246.4 s	6
08-19	129	7.4 s	90.6 s	278.1 s	8
08-27	106	7.6 s	18.0 s	176.6 s	2
09-02	126	8.4 s	120.3 s	156.9 s	11

Slow runs happened every day, and the worst maxima predate #2673 (2026-08-28). The budget did not create the slowness, it capped it — which is why max falls from ~250–278 s to ~121–157 s afterwards. Abandonment counts rise 4→15/day, but the first abandonment is 08-29, one day after the instrument was installed, so that rise is not evidence of a regression. Fleet control (the affected server excluded) is 0–13 over-60 s runs/day across ~5,000 daily runs with no trend, and 1 on 09-02.

No separate performance issue filed: claiming a trend off an instrument installed mid-window is the false-trend mistake.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MX6HyjsuDCs15qGB2rh4Gy

[claude]

This entry references [#2801] as a Markdown reference-style link, but there's no [#2801]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2801 definition added at the bottom of the file (where every other issue reference lives, e.g. [#2795], [#2791]). Without it, [#2801] will render as literal bracketed text instead of a link, unlike the rest of this section.

[claude]

Nit: every other format-string constant in this file (PartialDatabaseFailureNoteFormat, ProbeFailureNoteFormat, WallClockBudgetErrorFormat, UnreadableFailureSetErrorFormat) is paired with a cached CompositeFormat field to satisfy CA1863 ("parsed once"), but WholeCycleBudgetNoteFormat isn't, and both of its call sites (DarlingCollectorRunner.cs, RemoteCollectorService.DefinitionRunner.cs) go through string.Format directly. That's inconsistent with the established pattern here and will likely produce a CA1863 warning at both call sites, in tension with the CONTRIBUTING.md checklist item "C# code compiles without warnings."

[claude]

Coverage gap: this line (and DarlingCollectorRunner.cs's Abandoned: true return on budget expiry) has no direct test. Lite.Tests already has an end-to-end wiring test for the #2673 abandonment path (RunAsync_AnItemThatExceedsItsBudget_IsAbandoned_AndTheRestStillCollect), and this PR adds pure-function pins for ClassifyReturnedRun in Lite.Tests, but nothing in Darling.Tests exercises Darling's own wiring — that DarlingCollectorRunner actually sets Abandoned: true on budget expiry, or that this line turns it into status ABANDONED in the logged row. Given the whole point of this PR is that both hosts previously held their own hardcoded "SUCCESS" and silently drifted, it'd be worth a Darling-side regression test so this specific line can't silently regress the same way.

[claude]

Review summary

Focused, well-scoped fix: a single EnumeratedCollectorDriver.ClassifyReturnedRun shared by both hosts turns a #2673 whole-cycle-budget abandonment into its own ABANDONED status instead of the hardcoded SUCCESS each host previously wrote independently. Verified:

• Parity held: exactly one call site per host sets Abandoned = true (DarlingCollectorRunner.cs, RemoteCollectorService.DefinitionRunner.cs), and exactly one call site per host classifies it (DarlingWorker.cs, RemoteCollectorService.cs) — both going through the same shared EnumeratedCollectorDriver constant/method, so the two hosts can't drift on the status or the note wording again.
• State handling is correct: CollectorRunResult.Abandoned defaults false so the other 7 CollectorRunResult construction sites in DarlingCollectorRunner.cs are unaffected; Lite's mutable RunTelemetry.Abandoned is explicitly reset to false at the top of RunCollectorAsync, and the if (telemetry.Abandoned) override sits inside the success try block, so it can never clobber a status set by a catch (ERROR/PERMISSIONS/etc.).
• Downstream reads check out: ReadCollectionSignalsAsync and all four collection-health SUM(CASE WHEN status = ...) reads bucket by explicit allow-list, never by complement, so ABANDONED rows fall into no existing bucket rather than a wrong one — matching the PR's own reasoning. An abandoning collector now correctly ages out of last_success_time/staleness bands instead of reading perpetually fresh.
• Left 3 inline comments: a missing changelog link-reference definition for [#2801], a CA1863-cached-format inconsistency on the new WholeCycleBudgetNoteFormat constant, and a Darling-side test-coverage gap (the wiring in DarlingCollectorRunner.cs/DarlingWorker.cs has no direct test, unlike Lite's existing end-to-end abandonment test). None are blocking — the core logic and Lite/Darling parity are sound.

[claude]

This status flows into a complement-based bucket the PR's own design rationale says doesn't exist.

status here can now become "ABANDONED", which is then passed to RecordCollectorResult (line 737 below). That method (lines 305-348) is an if/else‑if/else chain: "SUCCESS" resets ConsecutiveErrors, "PERMISSIONS" and "YIELDED" get their own no-op branches — and everything else, including the new "ABANDONED", falls into the final else at lines 342-346, which sets LastErrorMessage and increments ConsecutiveErrors.

Before this PR, an abandoned cycle was hardcoded "SUCCESS" and hit the first branch (reset, healthy). After this PR, it hits the catch-all failure branch on the very first abandonment. GetHealthSummary treats ConsecutiveErrors > 0 as "erroring" (line ~256), and MainWindow.xaml.cs (lines 673-681) turns that into an orange-red "Collectors: N erroring" status-bar message with a tooltip literally labeled "Failing: {name}" — on a server slow enough to trip the wall-clock budget every cycle (per the PR body, abandonments went 4→15/day on the affected server), this immediately reads as the collector "failing" in Lite's live UI.

That's exactly the "would page on a guard doing precisely its job" outcome the PR rejected ERROR for — restated here for Darling's self-alert consecutive-failure path (EnumeratedCollectorDriver.cs doc comment), but Lite has its own, separate in-process consecutive-failure tracker that nothing in this PR accounts for. Darling has no equivalent per-collector in-memory tracker, so this is Lite-only — but real, and it undermines the whole point of introducing a non-paging status.

Suggest either adding an explicit "ABANDONED" branch in RecordCollectorResult that behaves like YIELDED (record the message, don't touch ConsecutiveErrors), or documenting why the Lite UI showing this as a failure is acceptable.

[claude]

Reviewed. The core change is well-designed: a shared EnumeratedCollectorDriver.ClassifyReturnedRun keeps Lite and Darling from drifting on the new ABANDONED status, both hosts thread Abandoned/CollectorRunResult.Abandoned through correctly, the note-format string is deduplicated into WholeCycleBudgetNoteFormat, status text/VARCHAR columns have no length constraint to worry about, and the six existing IN ('SUCCESS', 'SKIPPED') freshness reads across both hosts (DarlingSelfAlertEvaluator, ViewerDataService.CollectionHealth, DarlingFleetReader, DarlingDataReader, LocalDataService.CollectionHealth) already correctly exclude the new status, matching the PR's stated design.

One real bug found — see inline comment on Lite/Services/RemoteCollectorService.cs:631: Lite's in-process RecordCollectorResult health tracker (lines 305-348) is an if/else-if/else chain, and the new "ABANDONED" status falls into the catch-all else that increments ConsecutiveErrors and marks the collector "erroring" in the Lite desktop UI (orange-red status bar, "Failing: {name}" tooltip) — on the very first abandonment. That's the exact "pages like a fault" outcome the PR rejected ERROR for, restated in the shared driver's doc comment only for Darling's self-alert path. Darling has no equivalent in-memory per-collector tracker, so this is Lite-only, but it undermines the PR's own stated goal for the new status.

Two minor, non-blocking notes:

• The PR body/doc comments claim "every read buckets by explicit list ... never by complement." That's not quite true: ViewerDataService.Postgres.cs:66 has COUNT(*) FILTER (WHERE status <> 'SUCCESS') AS failed_runs. Effect is benign here (displayed status is LastStatus ?? "Failing", so ABANDONED just renders as itself), and this is likely the exact site the PR intends to "restore visibility" to — but it's worth tightening the claim since it also already counts YIELDED/SKIPPED as failed (pre-existing, unrelated to this PR).
• EnumeratedCollectorDriver.FreshnessSuccessStatuses (new) is only consumed by its own test (Lite.Tests/EnumeratedCollectorDriverTests.cs) — the six SQL sites still hardcode their own status IN ('SUCCESS', 'SKIPPED') literal independently, so the constant doesn't structurally prevent the four/six-way drift the doc comment says it protects against. Not a defect, just a gap between the doc's claim and what's actually wired up.

[erikdarlingdata]

All four review findings addressed in 3ccd56ae.

The RecordCollectorResult one was a real defect and it falsified this PR's own safety argument. I checked that every SQL read buckets status by explicit list — IN ('ERROR','PERMISSIONS'), = 'YIELDED', IN ('SUCCESS','SKIPPED') — and concluded a new value could not silently join the wrong bucket. I never checked the in-memory health path, which is an if/else-if/**else** chain, so ABANDONED landed in the error arm, set LastErrorMessage and incremented ConsecutiveErrors — showing the collector FAILING, precisely the false-error outcome this status was chosen to avoid.

It now has its own arm with the YIELDED semantics, which apply verbatim: the guard worked as designed so the streak must not grow, and nothing was collected so it is not proof the collector works and must not reset it either. I checked whether Darling has the same shape rather than assuming — it has no ConsecutiveErrors equivalent, so this was Lite-only.

CA1863 — parsed once now. Exposed as a method rather than the bare format string: unlike the four sibling constants, whose callers are all in-file, both hosts render this one, so publishing the string would leave each host to parse it and pick its own culture. That also removed the duplicated string.Format and the using System.Globalization each host had grown.

Darling.Tests coverage — added Runner_Marks_The_BudgetExpiry_Return_As_Abandoned. Your framing was the useful part: the worker classifies from the flag, not the note text, and those are separately editable, so a note reworded while the flag went unset would silently restore SUCCESS with nothing failing. Source-text pinned like the rest of that class, because the abandonment sits in a catch filter on a live provider cancellation that no unit test there can reach.

CHANGELOG — [#2801] link-reference definition added.

Separately, FleetIdentifierScrubTests caught a real fleet tenant name in the CHANGELOG and a doc comment. Fixed in 7e30328e, and I also scrubbed it from the issue and PR bodies, since this repository is public.

[claude]

Reviewed. This is a clean, well-scoped fix — no correctness, parity, or security issues found.

Verified specifically:

• CollectorRunResult.Abandoned / RunTelemetry.Abandoned both default false and are appended as the last parameter, so all 7+ existing positional construction sites are unaffected.
• Only the whole-cycle server-scoped budget-expiry catch sets Abandoned: true in both hosts (DarlingCollectorRunner.cs:1214-1232, RemoteCollectorService.DefinitionRunner.cs:785-798) — the separate per-database ([BUG] 3.4.0 query_store collector runs 37–100 min on Azure SQL DB (3.3.0 median: 4.8 s) — starves all other collectors #2150) fan-out abandonment path is untouched in both hosts, matching the PR's stated "deliberately not changed" scope.
• Lite's telemetry.Abandoned is reset to false alongside SqlMs/StorageMs/Note at the top of RunCollectorAsync, so no stale-state leakage across the per-server ConcurrentDictionary<int, RunTelemetry> reuse.
• RecordCollectorResult's new AbandonedStatus arm mirrors the YIELDED arm exactly (no streak growth, no streak reset) — correct, and matches the reasoning given in the PR description.
• Checked every consumer of collection_log.status for complement-based (<>/!=) reads that could silently misclassify the new ABANDONED value: DarlingSelfAlertEvaluator.ReadCollectionSignalsAsync and CollectorHealthClassifier.Classify's inputs (LocalDataService.CollectionHealth.cs, ViewerDataService.CollectionHealth.cs) all bucket by explicit list as claimed, so a collector abandoning every cycle now correctly ages into FAILING via the hoursSinceLastSuccess path rather than being miscounted as an error. The one place that is complement-based, ViewerDataService.Postgres.cs's failed_runs (status <> 'SUCCESS'), is explicitly the intended new visibility per the PR description, not a bug.
• Test source-text pins (Darling.Tests, Lite.Tests) match the actual code strings they assert against.
• No T-SQL touched by this PR, so the T-SQL style conventions don't apply here; the C# follows existing house patterns (CompositeFormat caching, XML doc style, shared-constant placement in EnumeratedCollectorDriver).

No inline comments to add.