Repository navigation
A scheduled analysis pass's failed family reads are now readable on get_collection_health instead of dying in a log line (#3691) - #3881
Merged
Conversation
…d, so a scheduled pass's caveats survive it (#3691) The scheduled sweep builds a fresh DarlingAnalysisService per pass, so LastCollectionFailures on the injected singleton only ever held the passes the MCP tools themselves started - which on a production install is none of them. A scheduled pass's collection caveats therefore lived in one Warning log line and reached no surface an operator or an agent could read. CollectionCaveatLedger is the process-wide home: a 24-pass ring per server, ConcurrentDictionary of rings with a lock per ring, every pass recorded including the clean ones (a clean pass is the only evidence a family recovered). Not persisted, by design - it describes the analysis pass's own reads of the store, so it carries its start stamp and says a restart forgets. Both SKUs' passes write it beside the properties they already set.
…s own failed reads (#3691) The ledger had no reader. get_collection_health is where an operator or an agent looks when the numbers on a server stop making sense, and it answered with 43 healthy collectors and no hint that the analysis pass above them timed out reading one of the families those collectors fill. The attach wraps the whole anonymous payload rather than adding a property to it, because McpHelpers.JsonOptions writes nulls: a property would ship analysis_caveats: null on every clean answer, on both SKUs, forever. Wrapping means the key exists only when the ledger has something to say. Placement is trailing, so nothing an existing consumer indexes by position moved.
…like, and both SKUs' descriptions say the key exists (#3691) Three surfaces, one fact. The sweep's AnalysisPassResult message is the row an operator reads in the analysis state, and a pass that could not read a fact family still RAN and still counted its findings honestly - it counted them over less evidence than the count suggests, and nothing said so. The caveat local is hoisted above both Ran arms so the window-empty arm composes rather than overwrites: that arm already carries a sentence, and a second fact about the same pass must not evict the first. The tool descriptions are pinned byte-identical across SKUs, so the appended sentence is the same bytes in both (20,263 characters each, verified equal after the edit) - it names every key the payload can carry, says which layer they describe, and says a clean window omits the key rather than nulling it, because an absent key otherwise reads as a surface that forgot to answer. Process memory is stated in the description itself: a reader who does not know a restart forgets will read a post-restart zero as evidence of health.
…rather than landing inside one (#3691) Part A appended the clause by string insertion and both landings split an existing sentence: Lite's read "come in two sizes and ; carries analysis_caveats when a recent analysis pass could not read a fact familythe mean describes neither", Darling's "repeat calls inside a minute ; carries analysis_caveats … familyread the memo rather than racing the scan". Both tables are what an agent reads to decide which tool to call, so a row whose prose collapses mid-clause is a garbled contract on the one surface that exists to be read. The clause now sits at the end of each row's prose, ahead of the closing pipe, and CollectionCaveatLedgerParityTests pins the shape both ways: the appended spelling is required, and the two mangled splices ("fact familyread", "fact familythe", "and ; carries") are asserted absent — a Contains on the clause alone passed on both broken rows.
…3691) Eighteen tests over the surface lane 45 added, in two classes. CollectionCaveatLedgerTests (13) exercises a ledger instance the suite constructs itself rather than CollectionCaveatLedger.Shared — the reason the class is a sealed class with a static well-known instance instead of a static class, because a suite that recorded into the process ledger would leave residue for every later test in the assembly and the ring-bound test alone plants twenty-four passes. Pinned: the ring holds 24 and overwrites rather than grows (30 recorded, 24 remembered, the newest at the head after the cursor wrapped); the denominator SATURATES, so a two-pass ledger says "2 of 2" and never "2 of 24" — a window it has not reached reads as an occasional blip when it is in fact every pass since startup; per-server rings, so one server's dark family never colours another's answer and an unrecorded server stays unknown rather than inheriting a ring; RecentPassCount counts PASSES carrying the family, once each even when two reads of it failed in the one pass, walking back from the newest, exact-match on the family, and answering 0 rather than throwing on the empty/zero/negative guards; a clean pass is RECORDED and says recovered, with entries_from STRICTLY OLDER than analysis_time (two stamps on one block, because one would have to either misdate the entries or hide the recovery) while failed_in_last_passes still says "2 of 3"; families_failed counts FAMILIES where entries stays one row per read; the seven-key top-level roster as an ordered set (analysis_time, families_failed, families_total, passes_remembered, recovered, entries_from, entries, finding, note — nine with the two the design line did not name) and the entry's five; Attach returning the VERY SAME reference with nothing owed, on both arms that owe nothing (no pass at all, and every remembered pass clean) and against another server's failure, so a clean answer cannot move a byte under a JsonOptions that writes nulls; the block appended AFTER every original key, so nothing read by position moved; the recorded failures being a COPY, proven by mutating the list the context still holds; and eight servers recording forty passes each while readers run, which is the contended shape (fifty servers' passes landing together) rather than a single-ring smoke. CollectionCaveatLedgerParityTests (5) reads source, the CollectionCaveatsParityTests discipline, because no CI-run project references both SKUs' assemblies and every invariant here is a WIRING fact no behavioural test can reach: each pass records exactly once, after its own instance stamp and BEFORE the failures-only report branch (so no arm returns before the pass is remembered, and a clean pass is recorded at all); each get_collection_health body wraps its payload through exactly one Shared.Attach( and never spells analysis_caveats as a property; the two descriptions carry the key, its rule and the restart sentence AND stay byte-identical to each other (20,263 bytes, verified — the twin-review invariant every earlier half-applied edit broke); both instruction rows carry the clause appended rather than spliced; and the scheduled pass COMPOSES its caveat into the window-empty message rather than overwriting it, with the Ran arms the only ones that gained one — a Skipped or TimedOut pass never finished its reads, so a caveat counted over it would describe a pass that did not happen.
This was referenced Sep 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What an operator now learns
Until this PR, a scheduled analysis pass that could not read a fact family said so in exactly one place: a Warning line in the service log. The pass recorded its failed reads on the context, the service copied them onto
LastCollectionFailures/LastCollectionFamilyCount, and the MCP tools rendering off that pass carriedcollection_caveats— which works for an MCP-triggered pass, because the tool and the pass share the injected service instance. It does not work for the scheduled sweep:DarlingWorkerbuilds a freshDarlingAnalysisServiceper pass, so the singleton the tools were handed remembers only the passes the tools themselves started. On a fleet whose passes are all scheduled — which is every production install — those properties are permanently empty, and the fact that eleven of twenty families timed out at 03:00 lived in a log line and died there.PARTIAL for #3691. The checklist line this completes, verbatim:
This is the WORKER half only. The Viewer half waits on a Windows build seat — stated on the face. Nothing under
PerformanceMonitor.Darling.Vieweris touched, so #3691 stays open.The ledger
PerformanceMonitor.Analysis/CollectionCaveatLedger.cs— a sealed class with a static well-known instance (CollectionCaveatLedger.Shared), theAlertReadFailureCounter.Sharedshape and deliberately so: the value is process-global by nature, the readers are static MCP tool methods in a DI container built separately from the one the pass runs in, and namingSharedat the write sites is what lets a test construct its own ledger and not pollute the process one — which apublic static classwould have made impossible.Record(serverId, serverName, analysisTimeUtc, failures, familiesTotal)on every pass, clean ones included. A clean pass is the only evidence that a family RECOVERED; recording failures only would leave the last failure looking current forever, which islast_error's last_error keeps a fault from a code path the collector no longer takes, with no timestamp to date it #3010 defect rebuilt one layer up. The failures are copied, because the list handed in is the live context's and a later failure must not appear inside an already-recorded pass.ConcurrentDictionary<int, Ring>with a lock per server (the contended case is fifty servers' passes landing together and none touches another's ring; readers copy under the same lock and compute outside it, so a slow serializer cannot hold a pass up). 24 is chosen against the shape of the question — "always failing, or once?" — at a roughly hourly sweep, so about a day. It is not a measured bar, claims no lineage, and nothing fires on it.StartedUtcinnoteand states outright that a restart forgets — thealert_read_health.counting_sincediscipline, which exists because a zero with no stamp beside it reads as reassurance. An absent block is never evidence that every pass was clean.The payload — seven top-level keys, plus the two the design line did not name
analysis_caveatsonget_collection_health, both SKUs:analysis_timefamilies_failed0when it was cleanfamilies_totalpasses_rememberedrecoveredlatest.Failures.Count == 0— the latest pass read every family and an older remembered one did notentries_fromanalysis_timeentries[]family,read,outcome,message,failed_in_last_passes("n of m")findingCollectionCaveats.Describe(...)— the same sentence the Warning log line and everyanalyze_serverpayload carrynoteStartedUtcTwo stamps on one block is the cost of not lying in either direction: the block describes the latest pass while the entries come from the newest failing one, so a recovery is readable as a recovery rather than as a current fault — and one stamp would have to either misdate the entries or hide the recovery.
failed_in_last_passesis stated against the passes the ledger holds, never against 24: a two-pass ledger says "2 of 2", because "2 of 24" reads as an occasional blip when it is in fact every pass since startup.The block is absent when the ledger has no snapshot for the server or every remembered pass was clean, and it is attached by wrapping the payload (
CollectionCaveatLedger.Shared.Attach(new { … }, resolved.ServerId, McpHelpers.JsonOptions)) rather than sitting in it as a property —McpHelpers.JsonOptionsWRITES nulls, so a literalanalysis_caveats = …would ship a null key on every healthy server's answer for a caveat nobody owed. With nothing owed the very same object reference comes back, so a clean answer cannot move a byte.analyze_nowDarlingWorker's pass result gains the caveat sentence on itsRanarms only: the caveat local is hoisted above the insufficient-data arm, the window-empty arm composes viaCollectionCaveats.Compose(it already had a sentence — an overwrite would trade one true sentence for another), and the findings arm passes it asMessage. ASkipped/TimedOut/Errorpass never finished its reads, so a caveat counted over it would describe a pass that did not happen.Tests
CollectionCaveatLedgerTests(13) — behaviour, over a ledger the suite constructs itself. Ring bound with the saturating denominator; per-server isolation;RecentPassCountcounting passes-not-reads withinlastN, exact-match on family, guards answering 0; clean-pass recording andrecoveredwithentries_fromstrictly older thananalysis_time;families_failedcounting families whereentriesstays per-read; the nine-key roster as an ORDERED set plus the entry's five;Attachreturning the same reference on both nothing-owed arms and appending after every original key otherwise; the failures being a copy (proven by mutating the list the context still holds); parallelRecordover eight servers × forty passes with readers running.CollectionCaveatLedgerParityTests(5) — wiring, read from source (no CI-run project references both SKUs' assemblies). Both passes record once, after their own stamp and before the failures-only branch; both tool bodies attach exactly once and never spell the key; both descriptions carry the key + rule + restart sentence and stay byte-identical (20,263 bytes each, verified); both instruction rows carry the clause; the worker composes rather than overwrites.Pre-checked green locally in an in-worktree harness (18/18 new;
CollectionCaveatsParityTests,RepoFileAdoptionTests,DocCommentHygieneTests82;McpPayloadContractCensusTests67;DarlingMcpToolsTests+AnalysisShutdownResidueTests23/1 skipped-live). Builds:PerformanceMonitor.Darling.ServiceandPerformanceMonitorLite, 0 warnings.Lite.Testscannot build on macOS (WPF), soServiceBuildSurfaceTests/AlertReadFailureSurfaceTests— the existing byte-identity andservice-block censuses over the twoget_collection_healthfiles — get their first execution on the Windowsbuildjob; their invariant was verified by hand instead (both descriptions 20,263 bytes, equal; noservicefield set changed).Also folded in
analysis_caveatsclause was spliced into the MIDDLE of an existing sentence (Lite/Mcp/McpInstructions.cs:60,Darling/…/DarlingMcpInstructions.cs:137) — Lite read "come in two sizes and ; carries analysis_caveats … fact familythe mean describes neither", Darling "repeat calls inside a minute ; carries … familyread the memo rather than racing the scan". Same lane, same two lines, and the tables are the one surface that exists to be read by an agent choosing a tool. Fixed to append at the end of each row's prose, andBothInstructionTables_CarryTheClauseOnTheirCollectionHealthRowpins both directions — the appended spelling required, the two mangled splices asserted absent, because aContainson the clause alone passed on both broken rows.Self-review
diff --stat(9 files, +912/−10): the ledger (321), its tests (543), two write sites (+11/+10), two attach sites (+11 each), two instruction rows (1 line each), the worker (+11/−2).Three riskiest hunks and their pins:
Attachwrapping the whole payload (DarlingMcpDataTools.cs:1229,McpHealthTools.cs:505) — it re-serializes the anonymous object to aJsonObjectand the result is serialized again by the caller. A clean ledger must return the identical reference or every healthy server's payload changes shape. Pinned byAttach_ReturnsTheVerySamePayloadReference_WhenNothingIsOwed(three arms) andAttach_AddsTheBlockBesideEveryOriginalKey_WhenABlockIsOwed(key order preserved, block last).analysis_timefor "when did this fail" is reading the wrong stamp on a recovered block. Pinned byACleanPassIsRecorded_AndSaysRecovered_WithTheEntriesDatedByTheirOwnPass(assertsentries_from < analysis_time, and the live arm asserts they are equal).Newest()silently reorders history, which every persistence figure is computed from. Pinned by the 30-pass wrap test (newest at the head), the saturating-denominator arm, andRecentPassCount'slastNwalk.Reviewer checklist: (a) is 24 the right depth for an hourly sweep, and is "not a measured bar" the honest label; (b) is
recoveredthe right flag name for "latest clean, older ones not"; (c)passes_rememberedandrecoveredare not in the design line's shape — keep them or drop them; (d) thenotesentence names the layer distinction (43 healthy collectors above a family the pass timed out reading) — is that the wording Erik wants on the face.What it does NOT do
The Viewer (waiting on a Windows build seat). No store table and no migration rung — the ledger is process memory by design and says so. It does not change what a caveat IS, and the Warning log line stays. No CHANGELOG entry in this PR.