Skip to content

A configuration lever hanging off a diagnostic chain is attached to that finding instead of orphaned into a card of its own, so a vacuum backlog and the memory bound that caps how fast it can clear arrive as one story (#3691) - #3850

Merged
erikdarlingdata merged 2 commits into
devfrom
fix/3691-walk-attaches-side-config-leaves
Sep 21, 2026

Conversation

@erikdarlingdata

Copy link
Copy Markdown
Owner

The greedy single-path story walk now attaches the config-advisory leaves hanging off its path to the story it belongs to, instead of leaving them to root cards of their own — so a vacuum backlog and the memory bound that caps how fast it can clear arrive as one finding with a lever, not two findings that never mention each other.

Partial for #3691. The checklist line this completes, verbatim:

The greedy single-path walk orphans config leaves not reachable from the path's tail (CONFIG_PG_MAINT_WORK_MEM roots alone at 0.6 beside a backlog → wraparound → hold chain) — engine item, both engines.

What was wrong

InferenceEngine.Traverse follows the SINGLE highest-severity active edge from each node until it runs out, and every node it lands on is marked consumed. That is the right shape for a story — a chain is a chain — but it means a node MID-path with two active edges reaches only one of them.

The vacuum chain is exactly that shape. PG_AUTOVACUUM_BACKLOG has an active edge to PG_WRAPAROUND_TREND (higher severity, and the hold chain behind it) and another to CONFIG_PG_MAINT_WORK_MEM — the dead-tuple memory each autovacuum pass may use, which bounds how much of the backlog one run can clear. The walk takes the wraparound. The knob is never visited, so nothing consumes it; and because it reaches 0.6, it then roots its OWN one-node story beside the incident it belongs to.

So the operator got two cards. One said "public.hot is 5× past its own autovacuum trigger line for four consecutive samples, and a held xmin horizon is why it cannot clear". The other said "maintenance_work_mem is small". Nothing on either card said they were the same problem, and the second one is the lever for the first — the one number you can change tonight.

The shape of the fix, and the one judgement call in it

After Traverse returns and its path is consumed, SweepSideLeaves walks the path once more and, for each node, reads that node's ACTIVE edges through the graph's own GetActiveEdges — the same call the traversal makes, so every predicate, and the bad-actor alias resolution the PostgreSQL graph performs, behave here exactly as they do in the walk. A destination is attached to the story as a SIDE LEAF, and marked consumed, when it is not on the path, not consumed by a prior story, scored above zero, and CONFIG-ADVISORY class.

The class test is by the fact's SOURCE, and that is load-bearing rather than tidy. The obvious test is membership of the advisory-root set (ConfigAdvisoryRootKeys / PgTargetFactKeys.IsConfigAdvisoryRoot) — and it would have missed the reported defect completely. That set answers a different question: "may this fact root BELOW the 0.5 incident line?" Its membership follows D5, so a CONVENTION check (a knob at its shipped default) is in it and an EVIDENCE-gated check is deliberately not — maintenance_work_mem may not root a card on a quiet server. But an evidence-gated knob that DID get its evidence is precisely the orphan here: CONFIG_PG_MAINT_WORK_MEM takes its 0.4 advisory base from the backlog ratio the vacuum collector stamps onto it, the backlog co-fire amplifier lifts it to 0.6, and it roots on the ORDINARY threshold without ever consulting the advisory-root set. So the sweep asks whether the fact came from a SETTINGS source — pg_config, pg_posture, or the SQL Server config / database_config collectors — unioned with the advisory-root set, which picks up the keys whose card is an advisory but whose fact rides a measurement source (MISSING_INDEX and PLAN_WARNING on queries, PLAN_CACHE_BLOAT on memory, CONFIG_PG_AUTOVACUUM_DISABLED on pg_vacuum). A source is the collector's own statement about what kind of thing it read, so it cannot drift the way a hand-kept key list does. I checked every emission site on those four sources: each one emits a setting, never a measured symptom.

Only config-advisory destinations, and this must not be widened. An incident-class side destination is a DIFFERENT story. It has its own symptom, its own advice, its own occurrence history, and ClusterIntoIncidents already re-merges it into one incident without taking its card away. A config advisory has no incident of its own — it is a standing setting whose entire claim to a card was that nobody else had said it. Widening the sweep to any destination would silently delete findings, which is why the class test carries the longest comment in the method.

Rejected alternatives. Appending the leaf to the path would have been three lines: it also changes StoryPath, and therefore StoryPathHash, and therefore every mute row, occurrence count and incident fingerprint for the chain — a re-identification of a live incident to make a card prettier. Letting the leaf keep its own card and linking the two cards leaves the "two cards" complaint exactly where it was. Following edges OUT of a side leaf would invent a second path, which is the thing this method exists to avoid.

Blast radius

AnalysisStory.SideLeafKeys and AnalysisFinding.SideLeafKeys are new, ephemeral, and not persisted — no schema change, no migration rung. Path, StoryPath, StoryPathHash, LeafFactKey and FactCount are untouched, so incident identity is byte-identical and every existing story pin holds unchanged (StoryNamedHopsTests and StoryConfidenceTests were executed against this build to prove it, not assumed). Severity and confidence are untouched: a lever is context, not corroboration, and amplifying is the graph's business.

The payload gains side_leaves on analyze_server only — one card per lever, carrying that key's advice so a reader told a lever exists does not need a second tool call to learn what it says. It is attached through StorySideLeaves.Attach, which returns the caller's payload object untouched when there is no lever. That is the CollectionCaveats.Attach discipline and it is deliberate: these tools' serializer writes nulls (the payload's own leaf_fact renders as null on a one-node story), so a side_leaves: null property would have been a byte change on every finding of every clean pass, for a thing that did not happen. get_analysis_findings is unchanged — it reads findings back from the store, which carries no side leaves, and inventing them on the read path would be a claim the store cannot support.

SQL Server output: byte-identical, structurally rather than by sampling. DB_CONFIG is the only config-advisory destination any SQL Server edge names; its only inbound edge is from LCK_M_S; and LCK_M_S declares exactly that one edge. So whenever the walk stands on LCK_M_S with DB_CONFIG fired, DB_CONFIG is the single candidate and goes ON the path, as it always did. No SQL Server story can carry a side leaf, so no SQL Server payload gains the key and no story text gains the sentence. This is pinned rather than asserted in prose, because it is the kind of claim that stops being true silently — the day someone adds a second edge out of LCK_M_S, or any edge into a CONFIG_* key, the pin fails and that becomes a review conversation instead of a surprise in a payload diff. (No planted SQL Server store had an orphaned config leaf today, which is the same fact seen from the other side.)

What it does not do

No graph edge changes. No severity, amplifier or confidence changes. No Viewer work — the viewer renders findings read back from the store, which carry no side leaves. No migration rung. No new knobs. get_analysis_findings unchanged. It does not follow edges out of a side leaf, and it does not attach incident-class destinations.

Test plan

New file Lite.Tests/StorySideLeafTests.cs, thirteen pins, all new: a config leaf off the MIDDLE node attaches as a side leaf, is consumed, and roots no second story (path, hash inputs, leaf and fact count all asserted unchanged); the same leaf off the TAIL is still walked onto the path as today; an incident-class side destination is not swept and still roots its own story; a side leaf at severity 0 is ignored; two levers off two nodes rank by severity with an ordinal tie-break; a lever consumed by an earlier, higher-severity story is not re-swept onto a later one; the real PostgreSQL vacuum chain with the real graph and real keys carries CONFIG_PG_MAINT_WORK_MEM as a side leaf — with Assert.False(IsConfigAdvisoryRoot(...)) asserting the premise, so a future edit to that list cannot quietly turn the pin into a tautology; the SQL Server impossibility above; and ToPayload / Attach / Sentence / PopulateStoryText emitting only when owed, including the byte-identity arm.

These were EXECUTED on this machine, per the campaign's execute-your-pins rule, through an in-worktree net10.0 xunit harness (deleted before commit) alongside the existing StoryNamedHopsTests, StoryConfidenceTests, TsqlConventionGuardTests, DocCommentHygieneTests and RepoFileAdoptionTests: 137 tests, 0 failures. Running them caught a defect compile-only would have shipped — the SQL Server pin's DB_CONFIG fact lacked the rcsi_off_count metadata the edge predicate actually reads, so the edge never fired and the assertion was passing for the wrong reason. Fixed and re-executed.

CI is still the first execution of these tests in the real suite, and the first execution of anything on the Windows side.

Builds, all zero warnings: PerformanceMonitor.Analysis, Lite (-p:EnableWindowsTargeting=true), PerformanceMonitor.Darling.Service, Lite.Tests, Darling/Darling.Tests, deprecated/Dashboard.Tests.

Which component(s) does this affect?

  • Lite
  • Darling
  • Lite Tests
  • Darling Tests
  • SQL collection scripts
  • Documentation
  • Full Dashboard (deprecated)
  • CLI Installer (deprecated)

How was this tested?

Thirteen new unit pins over the real InferenceEngine, the real RelationshipGraph and the real PgTargetRelationshipGraph, executed locally (137 tests including the neighbouring story suites and three repo censuses, 0 failures) and first executed in the real suite by CI. Pure engine logic: no server contact, no store contact, no fixture. The PostgreSQL scenario is pinned with the production keys and the production vacuum graph; the SQL Server no-change claim is pinned against the graph's own edge table rather than sampled.

Checklist

  • I have read the contributing guide
  • My code builds with zero warnings (dotnet build -c Debug)
  • I have tested my changes against at least one SQL Server version
  • I have not introduced any hardcoded credentials or server names

…haned into a card of its own, so a backlog and the memory bound that caps how fast it can clear are one finding (#3691)

The greedy single-path walk follows the highest-severity active edge from each
node, so a node mid-path with two active edges reaches only one of them. The
vacuum backlog reaches the wraparound trend — and the hold chain behind it —
and skips CONFIG_PG_MAINT_WORK_MEM, which is the dead-tuple memory bounding how
much of the backlog one autovacuum pass can clear: the lever that would relieve
the very thing the story is about. Never visited meant never consumed, and the
un-consumed knob then rooted its own one-node card at 0.6 beside the incident
it belongs to. The reader saw two cards where the truth is one story with a
lever, and nothing on either card said they were the same problem.

BuildStories now sweeps SIDE LEAVES after the traversal and before the story is
built: for every node on the path, each ACTIVE edge — read through the graph's
own GetActiveEdges, so every predicate and the bad-actor alias behave exactly as
they do in the walk — whose destination is not on the path, not consumed by a
prior story, scored above zero and CONFIG-ADVISORY class is attached to the
story as a side leaf and marked consumed. The advisory-class test is by the
fact's SOURCE (pg_config, pg_posture, config, database_config) unioned with the
advisory-root set, and that is load-bearing rather than incidental:
CONFIG_PG_MAINT_WORK_MEM is deliberately NOT an advisory root (D5 — an
evidence-gated knob may not root on a quiet server), it takes its 0.4 base from
the stamped backlog ratio and the co-fire amplifier lifts it past the ORDINARY
0.5 line, so a root-set membership test would have missed the reported defect
entirely. An incident-class side destination is untouched: it is a different
story with its own symptom, advice and occurrence history, and
ClusterIntoIncidents is what re-merges it into one incident without taking its
card away.

The leaves ride BESIDE the path, never in it. Path, StoryPath, StoryPathHash,
LeafFactKey and FactCount are what they were, so incident identity — the hash
every mute row, occurrence count and fingerprint keys on — is byte-identical and
no existing story pin moves. A side leaf does not lift severity or confidence
either: it is context, not corroboration, and amplifying is the graph's business.
The payload gains side_leaves, one card per lever with its advice, through an
Attach that returns the caller's object untouched when there is none — the
CollectionCaveats discipline, because the tools' serializer writes nulls and a
side_leaves: null would be a byte change on every finding of every clean pass.
The root's investigation gains ONE sentence naming the lever and pointing at its
card; a story with no lever freezes exactly what Compose returned.

The SQL Server pass cannot change, structurally rather than by sampling:
DB_CONFIG is the only config-advisory destination any SQL Server edge names, its
only inbound edge is from LCK_M_S, and LCK_M_S declares exactly that one edge —
so DB_CONFIG is always the single candidate and always goes on the path. A pin
holds that, and fails the day someone adds a second edge out of LCK_M_S.

Tests: Lite.Tests/StorySideLeafTests.cs, thirteen pins — a lever off the middle
node attaches and roots nothing; the same lever off the tail is still walked onto
the path; an incident-class side destination keeps its own root; a lever at
severity 0 is ignored; two levers rank by severity; a lever consumed by an
earlier story is not re-swept; the real PG vacuum chain with the real keys
carries maintenance_work_mem as a side leaf (with the not-an-advisory-root
premise asserted so the pin cannot become a tautology); the SQL Server
impossibility; and the payload/prose emitting only when owed. All thirteen were
EXECUTED on this machine through an in-worktree net10.0 xunit harness alongside
StoryNamedHopsTests, StoryConfidenceTests, TsqlConventionGuardTests,
DocCommentHygieneTests and RepoFileAdoptionTests — 137 tests, 0 failures. That
execution caught a defect compile-only would have shipped: the SQL Server pin's
DB_CONFIG fact lacked the rcsi_off_count metadata the edge predicate reads, so
the edge never fired and the assertion was passing for the wrong reason.
… caveat envelopes, so both SKUs must carry it exactly once and the two attachments cannot be confused
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant