Band a mid-sweep server Stale, not Offline: one shared collection-stopped window (Fixes #2794) - #2885
Conversation
…pped window (Fixes #2794) The display's freshness classifier called a server dark at a bare 15-minute constant while the alert engine had deliberately decided collection is not stopped until 30 - one condition, two definitions, and the tighter one false-alarmed by design: the sweep skips relaunch while a body runs, so one long query_store cycle legitimately leaves 12-19 minutes of silence on a healthy server. Measured first, per the issue's own instruction: post-#2792 the worst legitimate fleet gap in 24h is 12m12s and zero real servers cross 15 minutes (the sole >15m series is server_id 0, the service's own bookkeeping rows, breaking exactly at deploy restarts), while issue-day load produced 235 crossings in 12 hours peaking at 19m18s - and genuinely dark servers run hours. 30 minutes separates the populations with margin on both sides and is the number the alert engine already committed to. OfflineThreshold now derives from a shared CollectionStoppedMinutesDefault alongside every other spelling of the claim: DarlingSelfAlertEvaluator.StaleWindow, the AlertsConfig.CollectionStaleMinutes default, Lite's settings twin, AgentStatusRow.StaleWindow (a numerically-equal independently-editable copy - the #1562 drift shape), and the PG long-running-query recency bound whose own comment derives it from this convention. CollectionStoppedThresholdAgreementTests pins the agreement by value on both apps; proven red against the pre-change tree (15m != 30m, and the measured 19m18s stretch banding Offline). Co-Authored-By: Claude Sonnet 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}
| ### Fixed | ||
| - **A healthy server mid-sweep is no longer painted with the red Offline overlay** ([#2794]) - the display's freshness classifier banded a server `Offline - no recent collection` past a bare 15-minute constant, while the alert engine had deliberately decided collection is not "stopped" until 30 (`DarlingSelfAlertEvaluator.StaleWindow`, configurable as `collectionStaleMinutes`) - one condition, two definitions, and only the alert side configurable. The tighter one false-alarmed by design: the sweep skips relaunch while a server's collection body is still running, so one long `query_store` cycle holds the whole body and NOTHING writes to `collection_log` for the duration - a healthy server legitimately goes 12-19 minutes quiet with zero failures anywhere, and five production shards wore the red overlay at a snapshot while actively mid-cycle, which sent an investigation chasing phantom dark servers. **Measured before changing anything, per the issue's own instruction**: with [#2792] deployed, the worst legitimate inter-collection gap across the whole fleet in 24h is 12m12s and ZERO real servers cross 15 minutes (the sole >15m "server" is `server_id = 0`, the service's own bookkeeping rows, whose gaps end exactly at the deploy restarts) - but issue-day load produced 235 crossings on every server in 12 hours with legitimate stretch reaching 19m18s, so the margin between routine operation and the threshold was load-dependent and paper-thin, while genuinely dark servers run HOURS. `OfflineThreshold` now derives from a shared `CollectionStoppedMinutesDefault = 30` alongside every other spelling of the same claim - the evaluator's `StaleWindow`, `AlertsConfig.CollectionStaleMinutes`'s default, Lite's `AppAlertEngineSettings`, Lite's `AgentStatusRow.StaleWindow` (whose comment already said it "mirrors" the service's window, at a numerically-equal but independently-editable copy, the exact [#1562] drift shape), and `DarlingWorker`'s Postgres long-running-query recency bound, whose own doc comment derives it from the fleet staleness convention and which would otherwise have re-created the alert-blindness it warns about. A server between stretched sweeps now bands `Stale` - amber, "collection has lagged" - which is the honest reading; every surface follows because all four banding reads resolve through the one `ClassifyFreshness` ([#2473]), and `CollectionStoppedThresholdAgreementTests` pins the definitions together BY VALUE on both apps, proven red against the pre-change tree (15m != 30m, and the measured 19m18s stretch banding Offline). | ||
|
|
||
| ### Fixed |
There was a problem hiding this comment.
Nit: this creates two adjacent ### Fixed headers under [Unreleased] (one new at line 113 with just the #2794 entry, then this pre-existing one). Keep-a-Changelog style elsewhere in this file uses one ### Fixed section per version — worth merging the new bullet into the section below rather than adding a second header.
| /// running". Bounding "most recent" to a real window (rather than an unqualified <c>MAX(collection_time)</c> | ||
| /// that could resolve to a capture from hours ago) is what keeps an honest empty from silently going stale. | ||
| /// 15 minutes, matching the fleet's own <c>OfflineThreshold</c> staleness convention | ||
| /// The fleet's own <c>OfflineThreshold</c> staleness convention |
There was a problem hiding this comment.
The mechanical edit here (15 minutes, matching the fleet's own → The fleet's own) leaves this <para> without a subject/verb — it now reads "The fleet's own OfflineThreshold staleness convention ... rather than a tight multiple of the collector's 1-minute configured cadence — this fleet's delivered sweep cadence has been measured...", which doesn't parse as a sentence, and it also drops the only place that stated what recencyMinutes actually resolves to (30 minutes, via the shared constant). Compare with how the same rename was handled in DarlingFleetReader.cs (just dropped the "15-minute" adjective, sentence still parses) and in DarlingWorker.cs's PgLongRunningQueryRecencyMinutes doc comment (rewritten as full sentences) — this spot needs the same treatment, e.g. "Derives from the fleet's own OfflineThreshold staleness convention (...) rather than...".
|
Reviewed. This is a clean, well-scoped change — one shared constant ( Three minor doc/changelog nits, left as inline comments where the diff allowed it:
None of these affect runtime behavior. |
…e boundary ViewerServerSummaryDisplayTests carries its own copy of the band table (it pins ServerSummaryItem's delegation, not the classifier), and two ApplyFreshness facts fed 20- and 30-minute ages that now band Stale - exactly 30 sits ON the boundary and the strict > keeps it amber. Swept by shape this time: every ApplyFreshness/ClassifyFreshness call site in test code, not the three files the first pass found. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MX6HyjsuDCs15qGB2rh4Gy
| ### Fixed | ||
| - **A healthy server mid-sweep is no longer painted with the red Offline overlay** ([#2794]) - the display's freshness classifier banded a server `Offline - no recent collection` past a bare 15-minute constant, while the alert engine had deliberately decided collection is not "stopped" until 30 (`DarlingSelfAlertEvaluator.StaleWindow`, configurable as `collectionStaleMinutes`) - one condition, two definitions, and only the alert side configurable. The tighter one false-alarmed by design: the sweep skips relaunch while a server's collection body is still running, so one long `query_store` cycle holds the whole body and NOTHING writes to `collection_log` for the duration - a healthy server legitimately goes 12-19 minutes quiet with zero failures anywhere, and five production shards wore the red overlay at a snapshot while actively mid-cycle, which sent an investigation chasing phantom dark servers. **Measured before changing anything, per the issue's own instruction**: with [#2792] deployed, the worst legitimate inter-collection gap across the whole fleet in 24h is 12m12s and ZERO real servers cross 15 minutes (the sole >15m "server" is `server_id = 0`, the service's own bookkeeping rows, whose gaps end exactly at the deploy restarts) - but issue-day load produced 235 crossings on every server in 12 hours with legitimate stretch reaching 19m18s, so the margin between routine operation and the threshold was load-dependent and paper-thin, while genuinely dark servers run HOURS. `OfflineThreshold` now derives from a shared `CollectionStoppedMinutesDefault = 30` alongside every other spelling of the same claim - the evaluator's `StaleWindow`, `AlertsConfig.CollectionStaleMinutes`'s default, Lite's `AppAlertEngineSettings`, Lite's `AgentStatusRow.StaleWindow` (whose comment already said it "mirrors" the service's window, at a numerically-equal but independently-editable copy, the exact [#1562] drift shape), and `DarlingWorker`'s Postgres long-running-query recency bound, whose own doc comment derives it from the fleet staleness convention and which would otherwise have re-created the alert-blindness it warns about. A server between stretched sweeps now bands `Stale` - amber, "collection has lagged" - which is the honest reading; every surface follows because all four banding reads resolve through the one `ClassifyFreshness` ([#2473]), and `CollectionStoppedThresholdAgreementTests` pins the definitions together BY VALUE on both apps, proven red against the pre-change tree (15m != 30m, and the measured 19m18s stretch banding Offline). | ||
|
|
||
| ### Fixed |
There was a problem hiding this comment.
Nit: this adds a second ### Fixed heading directly after the one just added a few lines up (line 113), so the [Unreleased] section now has two consecutive ### Fixed sections instead of one. Every other entry in this section (and every prior release) uses a single ### Fixed heading per category. The new #2794 bullet should be merged into the existing ### Fixed block above rather than starting a new one.
|
Reviewed. This is a well-scoped, thoroughly-tested threshold change (15m -> 30m for "collection has stopped"), and the parity work is solid:
Left one inline nit: the CHANGELOG edit creates two consecutive No SQL in this PR (pure C#), so the T-SQL style conventions don't apply here. |
The inserted CHANGELOG bullet carried its own '### Fixed' heading directly above the existing one; it now joins the existing section. The PgSessionStatesReader recency comment lost its subject and the statement of what the parameter resolves to in the mechanical rename - rewritten as a sentence naming the shared constant. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MX6HyjsuDCs15qGB2rh4Gy
…hold-agreement # Conflicts: # CHANGELOG.md
|
Reviewed the diff. This is a small, well-scoped change: Checked:
One minor, non-blocking observation (not on a changed line, so noting here rather than inline): No blocking issues found. |
…ared CHANGELOG.md cited 1083 distinct [#NNNN] shortcut references against 1048 link-reference definitions, so 37 of them had nothing to resolve to and GitHub rendered them as literal "[erikdarlingdata#1696]" bracket text instead of links - in prose where the reference IS the citation, e.g. "[erikdarlingdata#2760] removed both hints on the theory that..." reads as a dead footnote. Appended the 37 missing definitions in the existing form. Nothing in the existing block was reordered or deduped: the duplicate [erikdarlingdata#828] and [erikdarlingdata#887] definitions are left exactly as they were, and the file's own bytes are unchanged ahead of the append - the diff is 37 insertions and no deletions. All 37 numbers were confirmed to exist in the repo (33 issues, 4 pull requests). They are written uniformly as /issues/NNNN, which is what recent practice already does for both kinds - the /pull/ form stops at erikdarlingdata#2770, six of the 60 most recent /issues/ definitions are PRs, and GitHub redirects /issues/N to /pull/N anyway. Pre-existing, and found incidentally while landing erikdarlingdata#2885 rather than caused by it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Fixes #2794.
One condition, two definitions
The display's freshness classifier banded a server
Offline - no recent collectionpast a bare 15-minuteOfflineThreshold, while the alert engine had deliberately decided collection is not "stopped" until 30 minutes (DarlingSelfAlertEvaluator.StaleWindow, configurable ascollectionStaleMinutes). The tighter definition false-alarmed by design: the sweep skips relaunch while a server's body is still running, so a single longquery_storecycle holds the whole body and nothing writes tocollection_logfor the duration - a healthy server legitimately goes quiet for 12-19 minutes with zero failures anywhere, and five production shards wore the red overlay at one snapshot while actively mid-cycle.Measured before changing anything (the issue's own instruction)
The issue said: deploy #2792 first, re-measure, and only change the classifier if gaps still threaten the threshold. Measured on the production store over the last 24h (post-#2792 and the subsequent fetch/timeout fixes), via
LAG(collection_time) OVER (PARTITION BY server_id ORDER BY collection_time):The only >15m series today is
server_id = 0- the service's own bookkeeping rows, absent fromcollect.servers, whose gaps end exactly at the deploy restart stamps. So the 15-minute band currently produces zero false Offlines but sits 2.8 minutes above the routine worst case, issue-day load blew through it fleet-wide, and genuinely dark servers run hours (the incidents Offline exists for). 30 minutes separates the two populations with real margin on both sides - and it is the number the alert engine already committed to.The change
ServerHealthThresholds.CollectionStoppedMinutesDefault = 30is now the ONE default for "collection has stopped", and every spelling of that claim derives from it:OfflineThreshold(display band, all four collection-health reads have no count for the ABANDONED status #2804 surfaces via the singleClassifyFreshnessper The Darling viewer's sidebar dot and its Overview card disagree about a never-collected server #2473)DarlingSelfAlertEvaluator.StaleWindowAlertsConfig.CollectionStaleMinutes's default (the config can still widen the ALERT window; the display stays at the shared default, which is the conservative direction - the band can only be tighter than the alert, never looser)AppAlertEngineSettings.CollectionStaleMinutesandAgentStatusRow.StaleWindow(whose comment already claimed to "mirror" the service's window, at a numerically-equal but independently-editable copy - the exact Darling Web: a browser viewer served by the service (Mac/cross-platform), riding the existing Kestrel + server-side read layer #1562 drift shape)DarlingWorker.PgLongRunningQueryRecencyMinutes, whose own doc comment derives it from the fleet staleness convention; leaving it at 15 while Offline moved to 30 would re-create the alert-blindness that comment warns about, on exactly the busiest serversA server between stretched sweeps now bands
Stale(amber, "collection has lagged") - the honest reading the enum already had.Pins, proven red
CollectionStoppedThresholdAgreementTests(both apps) holds the definitions together by value across the seams a shared constant cannot reach (int config default vs TimeSpan window vs TimeSpan threshold). Proven red three distinct ways against the reverted tree via anet10.0harness reading the built assembly (printed threshold confirming each rebuild took: 30 -> 15 -> 30):Offlineon the old tree,Staleon this oneBoundary tests updated to the new band edge (
ServerHealthClassifierTestsband table now names 19m18s as the must-never-band-Offline case; the sidebar-dot Offline exemplars move from exactly-30m - which the strict>now bands Stale - to 31m). Lite'sServerCollectionFreshnessTestsare already fully symbolic overOfflineThresholdand pass unchanged, including the rendered "over N minutes" copy.Read-side only: no schema rung, no
StorageVersion.SchemaVersionchange, no change to what collectors write, no per-surface edits - the four banding surfaces stay consistent by construction since they share the one classifier.🤖 Generated with Claude Code
https://claude.ai/code/session_01MX6HyjsuDCs15qGB2rh4Gy