Fixes #2902

[erikdarlingdata]

Fixes #2902.

The design question first: (a), the keying is the bug — not (b), the accounting

The issue asked whether cross-collector draining is deliberate (in which case the defect is the
attribution, a bigger change) or accidental. It is accidental, and the evidence is in the tree:

• FetchAndStorePlansAsync's own doc calls it "the activity-driven plan-XML fetch for one
  database" (query_store collector costs 40-110s per run on multi-tenant primaries — the read has no interval watermark, so every cycle pays for the whole window #2312 Finding 2). No collector-sharing contract is stated or implied.
• The call site says the opposite of sharing: "this fetch is Query-Store-only, so the pattern narrows
  the type the signature needs AND gates the engine in one expression".
• ExtractPlanReferences' doc records the mistaken belief directly: "any batch that is not
  query_store rows extracts nothing, and the caller's CapturePlanXml gate means that never actually
  happens." It happens on every cycle — CapturePlanXml is a SKU flag set once per cycle from
  _capturePlans(), so it gates the SKU, not the collector. That comment is corrected in this PR.
• query_store plan/text fetch goes activity-driven: the store is the watermark (#2312) #2333's honesty notes enumerate every deliberate trade the carryover makes — process-memory,
  restart forgets the debt, the plan-dimension growth bend — and cross-collector draining is not among
  them. In a PR that documents its trades that carefully, an intentional shared queue would have been
  named.
• git blame: both fields landed in one commit, 8c79f0cb (query_store plan/text fetch goes activity-driven: the store is the watermark (#2312) #2333), with the key shape copied from
  _consecutiveQueryStoreItemFailures — per-DATABASE for the same reason, and safe that way only
  because every one of its call sites sits behind an explicit definition.Name == query_store check.
  The shape was inherited; the guard was not.
• No test and no doc anywhere in the repo mentions carryover. An intentional cross-collector contract
  with no pin, in this codebase, is not plausible.
• The fetch runs QueryStoreCollector.Instance.BuildPlanFetchByIdsQuery whichever collector is
  executing. A shared queue would not be built out of one collector's singleton.

Nothing was found on the (b) side beyond the abstract "debt paid sooner is better", which no comment
makes.

Two corrections to the issue

1. The text side is not gated by CapturePlanXml at all. FetchAndStoreQueryTextAsync is gated on
   context.FetchQueryTextSeparately, which the cycle context sets to a hardcoded true ([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). So
   text carryover was drained cross-collector even with capturePlans off. The plan side is as the issue
   describes.
2. The family is five, not three. database_scoped_config also reaches the fetch; it does not appear
   in the measurement because it is on-load only (FrequencyMinutes 0), so it almost never arrives while
   debt is outstanding. The three the log caught are exactly the three that run on a cadence.

The fix

carryKey is one variable feeding four per-(server, database) fetch-state dictionaries, so all four
widen together rather than two of them growing a near-identical sibling key:

• _planFetchCarryover, _textFetchCarryover — the deferred-id debt (the issue's subject).
• _observedPlanSize (learned plan size + ConsecutiveFetchFailures) and _textFetchFailures — whose
  per-database counters those same foreign visits were also resetting, on the nothing-owed return
  path that calls ClearPlanFetchBackoff / TryRemove before returning. Behaviour is otherwise
  unchanged for them, because after this only query_store reaches a fetch that reads or writes any of
  the four.

FetchStateKey is a method rather than an inline tuple literal so the pin drives the shipped
construction — an inline literal at each site is how the collector went missing in the first place.

No schema change, no migration: the per-phase persistence rung is #2860's and comes after this.

What this does and does not change

It saves no work and moves none. The same ids are still fetched and the same bytes still ship;
query_store's own measured cost rises by roughly what its siblings stop being charged. It is an
attribution fix, worth making because #2847, #2841 and #2860 all read collection_log per collector, and
this moved time between collectors and between cadence tiers in the series they reason from.

Deferral latency. Debt now waits for query_store's next visit rather than the first of five. At the
CollectorScheduleDefaults cadences that is at most one 5-minute cycle: query_store and
plan_correction are both on the 5-minute tier, while query_store_health is hourly,
index_object_stats daily and database_scoped_config on-load. So the drain opportunities that
materially existed were the two 5-minute ones, and this roughly halves them.

The delivered cadences say the same thing and are the ones that matter, since the sweep runs well behind nominal at fleet size. Two days of the tool's own per-collector aggregate on 42 servers: query_store 15,485 runs and plan_correction 15,586 — 7.81 and 7.76 minutes per server per run, effectively identical — against query_store_health's 1,731 (69.9 min) and index_object_stats' 45 (~44 h). Summing per-minute drain opportunities, this drops them by ~52% and adds a median wait of about 4 minutes, bounded by one query_store cycle. Nothing downstream reads the debt: it is process-memory with no reader outside these two methods, and the plan XML it eventually lands is an enrichment the readers already fall back from.

Eviction: there is none, and that is worth stating plainly. _planFetchCarryover[carryKey] = owed
stores the whole remaining set. Ids leave only by landing, by proving target-side-gone on a provably
uncut pass, or with the process. No count cap, no byte cap, no age-out. The declared bound is the field's
own doc — "ids are 8 bytes and a first-contact backlog is one catalog's worth" — which is a ceiling, not
a cap. So halving the 5-minute-tier drain rate makes an existing capacity question visible rather
than creating one: if query_store's own candidate cap cannot cover what its probe finds missing, the
backlog grows, and that was already true with plan_correction's free passes masking it. #2860's
persisted phase columns (ids attempted against probe ids per query_store run) are the instrument to
watch it with; nothing else can see it today.

The pin

PlanFetchCarryoverKeyTests, five assertions, behavioural rather than shape — "the key has three fields"
would pass for a key carrying a collection time, which fixes nothing and drops every deferred id.

• The family is derived by invoking the definitions' own RunsPerDatabase / BuildEnumerationQuery
  over CollectorCatalog.All, in the shape ServerWatermarkDispatchGateTests established. The only name
  written by hand is the owner, and that is structural (the fetch runs QueryStoreCollector.Instance's
  builders regardless of who executes) — and it is itself checked, since the owner must be a member of
  the derived family.
• The artifacts under test are the dictionaries' own declared key type (a fresh instance of
  FieldInfo.FieldType) and the runner's own key builder, both reached by reflection so that a
  key-shape change goes red instead of failing to compile. A pin that stops compiling when the thing it
  guards changes is worth nothing.
• BothFetchBodies_BuildTheirKeyThroughTheOneBuilder... closes the tautology hole: it reads the built
  assembly's IL on The IL-scan idiom in four reachability pins is unsound: an opcode-shaped byte scan that skips, and a token map that cannot see generic callees #2898's shared decoder, per state machine, with TouchAndProbePlansAsync /
  TouchAndProbeTextsAsync as positive controls checked first, and asserts each of
  <FetchAndStorePlansAsync> and <FetchAndStoreQueryTextAsync> calls FetchStateKey.

Red-first, five variants, five different assertions, each a mutation of the shipped source, rebuilt
and re-run:

Mutation	Assertion that went red
revert the key to (ServerId, Database)	NoOtherCollectorOnTheFetchPath_CanDrainTheOwnersDeferredIds — "database_scoped_config finds query_store's deferred ids in _planFetchCarryover under its own key"
key unique per call (collectorName + Guid)	TheOwningCollector_StillFindsItsOwnDeferredIds... — "query_store cannot find the ids it just deferred"
key drops the database	TheKeyStillSeparatesServersAndDatabases... — "Two databases on one server share a _planFetchCarryover entry"
re-inline the key at the text fetch	BothFetchBodies_BuildTheirKeyThroughTheOneBuilder... — "<FetchAndStoreQueryTextAsync> never calls FetchStateKey"
query_store_health stops enumerating	TheFetchPathFamily_HoldsTheOwnerAndSeveralOthers... — "Only 4 collectors reach the plan/text fetch"

Verification, and its scope

• Against the built assembly, not the source. Reflected out of
  PerformanceMonitor.Darling.Service.dll: all four fields now key on
  ValueTuple\3<Int32,String,String>with tuple element names[ServerId, Database, Collector], and both fetch methods carry a collectorName` parameter. Proven non-stale by a control: applying the
  revert and rebuilding flips the reflected arity to 2 and changes the MVID; restoring flips it back.
• Whole solution compiles (-p:EnableWindowsTargeting=true), including Darling.Tests.
• The five tests were executed on macOS by compiling the real test source (never a copy) plus
  IlCallSiteScanner.cs into a throwaway net10.0 xunit host with AssemblyName=Darling.Tests so
  InternalsVisibleTo applies: 5 passed, 0 failed.
• Not verified here: anything only the Windows suites or a live fleet can settle. The pin cannot
  prove the call site passes the cycle's collector rather than a constant — only a sweep can, and the
  test class says so in its own scope note. The ~346 s figure is the issue's log parse, not re-derived.

Corroboration from the store, independent of the log parse

Two days of the tool's own per-collector aggregate on the 42-server fleet says the same thing from the
other side: query_store_health reads one cheap row per database per hour and averages 304 ms over
1,731 runs — with a 72,454 ms worst execution in the same window. A 238x tail on a collector whose
own read cannot produce one. database_scoped_config, which is on-load only and so rarely arrives while
debt is outstanding, tops out at 562 ms.

🤖 Generated with Claude Code

[claude]

Reviewed the diff (CHANGELOG.md, DarlingCollectorRunner.cs, PlanFetchCarryoverKeyTests.cs) against CONTRIBUTING.md conventions and the correctness/parity/security/perf checklist. No T-SQL is touched, so the T-SQL style section doesn't apply here.

Correctness

• All four widened dictionaries (_observedPlanSize, _planFetchCarryover, _textFetchCarryover, _textFetchFailures) are read/written exclusively through the new carryKey/FetchStateKey(serverId, database, collectorName) — no leftover call site still builds the old 2-tuple (ServerId, Database) shape for any of them. Confirmed by grep.
• definition.Name is correctly threaded into both FetchAndStorePlansAsync and FetchAndStoreQueryTextAsync call sites, and the parameter position lines up with each method's new collectorName parameter.
• The untouched _consecutiveQueryStoreItemFailures dictionary correctly stays 2-tuple-keyed — its three call sites are all still guarded by an explicit definition.Name == query_store check, so it doesn't have the same latent bug and didn't need widening.
• Verified the "quiet cycle" early-return path (references.Count == 0 && !hasCarryover) still fires zero-store-read/zero-target-query for the four non-owner collectors post-fix — they now each get their own always-empty carryover entry instead of accidentally finding query_store's, so the fix actually eliminates the misattributed work rather than just relabeling it.
• Tuple default equality for string is ordinal, matching the StringComparison.Ordinal used elsewhere in this file for definition.Name comparisons, as the doc comment claims.

Lite/Darling parity

• Confirmed via search that Lite has no equivalent plan/text fetch carryover mechanism (CapturePlanXml, FetchQueryTextSeparately, budget-capped per-id deferral) — Lite's Query Store collection uses a durable last_execution_time watermark instead of an in-memory deferred-id queue. A comment in Lite/Database/DuckDbInitializer.cs explicitly notes CapturePlanXml is Darling-only. So this bug and its fix are architecturally moot for Lite — no parity drift introduced.

Tests

• PlanFetchCarryoverKeyTests derives its collector family from CollectorCatalog.All rather than hardcoding names (so a 6th enumerating collector is covered automatically), pins behavior rather than key shape, and includes an IL-level check that both fetch bodies actually call the shared FetchStateKey builder rather than re-inlining a tuple literal. IlCallSiteScanner (the shared decoder it depends on) already exists from The IL-scan idiom in four reachability pins is unsound: an opcode-shaped byte scan that skips, and a token map that cannot see generic callees #2898.
• FetchStateKey is internal, and InternalsVisibleTo for Darling.Tests is already present in the service .csproj.

Style

• New file has the required copyright header. Naming and doc-comment conventions match the rest of the file. CHANGELOG entry follows the existing dense, citation-heavy format and the [#2902] link reference was added correctly at the bottom.

Nothing to flag — this looks like a solid, well-scoped attribution fix with good test coverage for the failure mode it's closing.