Serialize the Query Store tick and backfill per server (#2165)

[erikdarlingdata]

Closes #2165.

What was measured

The per-tick query_store collection and the #2058 first-contact backfill were independent loops with no per-server coordination at all, and both do heavy Query Store text extraction. From the dogfood box (4-core, multi-tenant, mid-consolidation):

a 64MB backfill slice for the freshly restored database ran concurrently with the tick's QS collection of a sibling database (12:50:58 backfill ship overlapping the 12:51:09 tick completion) — two heavy PerformanceMonitorLite statements at once, ~128MB of QS text extraction in flight on the box least able to afford it.

The overlap isn't bad luck. A big catalog arriving is exactly what triggers both the backfill and budget-bound tick passes, so the two loops are most likely to collide precisely when the server is already drowning.

The gate

One shared QueryStoreServerGate in PerformanceMonitor.Common, sitting beside AbandonableStep — that one bounds how long a step may hold a loop, this one bounds what may run beside it. Each host keeps its own keyed registry (Darling by int server id, Lite by string), because the primitive is the gate, not the registry.

Wired at two points per app:

• Darling — in RunOneAsync, the one funnel that has the runtime and the collector name together; and around the backfill slice in RunQueryStoreBackfillLoopAsync.
• Lite — inside CollectQueryStoreAsync rather than at the tick's dispatch switch, so every caller is covered including an on-demand collection; and around the slice in RunQueryStoreBackfillTickAsync.

Three decisions worth calling out

Nothing ever waits. Both sides try-acquire with a zero timeout and skip on failure. These are shared fleet loops and an in-flight slice runs to a 180-300 second abandonment deadline, so a blocking acquire would let one slow server stall collection for every other server — the #2148 wedge arriving through a lock instead of a hang. A gate that can only skip cannot do that.

Skipping is safe for this collector specifically, and that's load-bearing rather than incidental. Query Store collection is watermark-driven (#1960): each pass resumes from the last shipped boundary, so a skipped pass defers rows, it doesn't drop them. Documented on the type, because the same gate around a wall-clock-windowed collector would silently lose data.

"Tick wins, backfill defers" is realized by cadence, not preemption. The loser is whichever loop arrives second — stopping a statement already running against the monitored server would mean killing it, and cancelling a QS read mid-flight buys nothing that waiting one cycle doesn't. The bias comes from the tick retrying on its ~1-minute interval against the backfill's 5, so it recovers ~5x faster from a collision, and a slice is byte-budgeted so it's short in the healthy case.

Two smaller ones:

• The backfill takes the gate outside its AbandonableStep, so an abandoned-but-still-wedged slice keeps the gate closed — that statement is genuinely still running on the server, so the tick must keep yielding to it. Pinned by index comparison, not by comment.
• An interlocked flag, not a SemaphoreSlim. A gate that never waits needs none of what a semaphore provides (blocking acquire, timeouts, async waits) and would otherwise own one undisposed kernel-backed object per monitored server for the process lifetime — which is also what CA1001 flagged on the first cut. Leases are idempotent on dispose, because a stray second Dispose() would clear a flag the other loop had since taken and let both run at once: the exact condition being prevented, reached from the wrong direction.

Testing

Both suites are net10.0-windows, so beyond the xunit tests I compiled the gate into a plain net10.0 harness and executed the semantics here — 17/17 pass, including 8-way contention over 16,000 attempts:

Contention: at most one holder, ever
  PASS  max concurrent holders = 1 (must be 1)
  PASS  actually acquired 7595 times
  PASS  gate free at the end
Never waits
  PASS  1000 refused acquires took 0ms (<1000)
Double-dispose cannot steal someone else's hold
  PASS  backfill's hold SURVIVES the tick's double-dispose

Covered: exclusion; release handing over; refused acquires returning immediately (asserted on elapsed time, because a blocking acquire would still pass an exclusion-only test while reintroducing the fleet stall); double-dispose not stealing another loop's hold; max-one-holder under contention; the NotGated sentinel being non-null and re-disposable; and per-server independence — a single shared gate would serialize QS collection fleet-wide, a throughput regression dressed as a fix.

The wiring is pinned at the source in both apps, because behavioral coverage can't reach it — reproducing the overlap needs two live loops against one real server with a big catalog, and a correct gate that one loop doesn't take is exactly the bug still present while everything else passes. Those pins assert the registry is declared once per app (two loops each holding a private registry would compile, pass every gate unit test, and exclude nothing), that Darling gates on QueryStoreCollector.Instance.Name rather than a literal so renaming the collector can't silently unhook it, and that the backfill takes the gate before the step. I ran all 11 pin assertions plus the doc-comment hygiene detector locally — all pass.

Full solution builds clean, 0 warnings.

🤖 Generated with Claude Code

[claude]

The gate is released on Abandoned, even though the guarded statement is still running. This contradicts the stated design ("held for the WHOLE slice... an abandoned-but-still-wedged slice keeps the gate closed") and the PR description's testing claims.

using var backfillGate = gate; disposes at the end of this loop iteration's block — i.e., right after the switch below, regardless of result.Outcome. But per AbandonableStep.RunAsync (PerformanceMonitor.Common/AbandonableStep.cs:132-143), Abandoned means the underlying task (work) is still executing in the background; the step's own _inFlight guard is only cleared later, in work.ContinueWith when the task truly ends.

So on abandonment here:

1. backfillGate disposes immediately, releasing the per-server QueryStoreServerGate.
2. The zombie backfill statement keeps running against the monitored server.
3. On the very next tick (~1 minute later), RunOneAsync's TryAcquire() on the same gate succeeds and launches a fresh query_store collection against the same server.

That's exactly the concurrent-heavy-QS-extraction scenario this PR exists to prevent — and it's most likely to happen precisely when a slice is large enough to hit the 300s abandonment deadline, i.e. exactly the case that matters most.

The gate's lease needs to be handed to the abandoned task and released only when it truly finishes (e.g. threaded through AbandonableStep's own completion, similar to how _inFlight is cleared in work.ContinueWith), not disposed based on RunAsync merely returning.

Note the new Darling_BothLoopsShareOneGateRegistry test only asserts textual ordering ("gate acquired before the step") — it doesn't exercise an actual abandonment, so it doesn't catch this.

Same pattern exists in the Lite counterpart (RemoteCollectorService.QueryStoreBackfill.cs), so this isn't a parity drift — it's a shared bug in both apps.

[claude]

Same issue as the Darling counterpart (DarlingWorker.cs, RunQueryStoreBackfillLoopAsync): using var backfillGate = gate; releases the gate at the end of this loop iteration, which happens as soon as step.RunAsync returns — including on AbandonableStepOutcome.Abandoned, where the underlying task is still running (see AbandonableStep.RunAsync, PerformanceMonitor.Common/AbandonableStep.cs:132-143: work keeps executing; _inFlight only clears later via work.ContinueWith).

So a wedged/slow backfill slice that trips the abandonment deadline releases the gate immediately, and the tick's CollectQueryStoreAsync can then acquire it and run query_store collection concurrently with the still-running backfill statement — the exact overlap this PR is meant to eliminate, and most likely exactly when a slice is big enough to be abandoned in the first place.

The lease needs to transfer to the abandoned background task and release only when that task truly completes, not when RunAsync returns control to the loop.

[claude]

Review

This is a clean, well-scoped PR — the QueryStoreServerGate primitive itself (interlocked flag, idempotent lease disposal via Interlocked.Exchange(ref _gate, null)?.Release(), non-null NotGated sentinel) is correct and matches the codebase's style. Parity between Darling and Lite is good: both wire the gate at the equivalent funnel (Darling's RunOneAsync, Lite's CollectQueryStoreAsync) and both take the gate outside the backfill's AbandonableStep. No SQL injection, secrets, or file/network/process concerns — this PR touches no T-SQL and no external I/O beyond what already existed.

One correctness bug, left as inline comments, present identically in both apps:

In both DarlingWorker.RunQueryStoreBackfillLoopAsync and RemoteCollectorService.RunQueryStoreBackfillTickAsync, the gate lease (using var backfillGate = gate;) disposes at the end of the current loop iteration — i.e., as soon as step.RunAsync(...) returns. But AbandonableStep.RunAsync can return Abandoned while the guarded task (work) is still running in the background (its own _inFlight guard only clears later, via work.ContinueWith, when the task truly ends).

That means exactly the scenario this PR is built to prevent — a large/slow Query Store slice overlapping the tick's own Query Store collection — is the one scenario where the fix silently doesn't hold: an abandoned slice releases the gate immediately, and the very next tick can acquire it and run concurrent heavy QS extraction against the same server, recreating the original bug. This directly contradicts both the PR description's claim ("the backfill takes the gate outside its AbandonableStep, so an abandoned-but-still-wedged slice keeps the gate closed") and the in-code comments making the same claim.

The new Darling_BothLoopsShareOneGateRegistry / Lite_BothLoopsShareOneGateRegistry tests only assert textual ordering (gate acquired before the step is constructed) — they don't exercise an actual abandonment, so this gap isn't caught by the added test suite despite the PR body describing thorough behavioral coverage of the gate's contention/dispose semantics.

Suggested fix direction: transfer the gate lease to the abandoned background task (e.g., dispose it inside AbandonableStep's own completion continuation, mirroring how _inFlight is cleared) rather than disposing it based on RunAsync merely returning control to the loop.

[claude]

The gate is released as soon as RunAsync returns, not when the slice truly finishes — which reopens exactly the overlap this PR exists to close.

AbandonableStep.RunAsync returns AbandonableStepOutcome.Abandoned while work keeps running in the background; the in-flight guard only clears later, inside work.ContinueWith(...), when the task actually ends (see AbandonableStep.cs lines 101-131). backfillGate here is a using var scoped to this foreach iteration. When the 300s deadline fires, the switch below logs Abandoned and falls off the end of the iteration — disposing backfillGate immediately, while the abandoned backfill statement is still executing against runtime's server.

The very next tick can then acquire the now-free gate (RunOneAsync, line ~3925) and start query_store collection against that same server while the wedged backfill slice is still running underneath it — the ~128 MB-concurrent-extraction scenario from the PR description, reintroduced specifically in the one failure mode (a wedge) where it's most damaging.

The doc comment right above ("an abandoned-but-still-wedged slice keeps the gate closed") and the Abandoned log message both describe behavior the code doesn't actually have.

AbandonableStep doesn't currently expose a "truly finished" hook to release into — only onLateFault, which fires solely on exceptions after abandonment, not on a clean late completion. Fixing this needs either a new completion callback on RunAsync, or capturing work and disposing the gate lease in a ContinueWith on it, rather than via using on the synchronous return path.

Same pattern, same bug, in the Lite twin: Lite/Services/RemoteCollectorService.QueryStoreBackfill.cs around line 99.

[claude]

Same issue as the Darling twin — the gate releases when RunAsync returns, not when the slice truly finishes.

AbandonableStep.RunAsync returns AbandonableStepOutcome.Abandoned after the 180s deadline while work (RunQueryStoreBackfillSliceAsync) keeps running in the background — the in-flight guard only clears later, inside work.ContinueWith(...), when the task actually ends (AbandonableStep.cs lines 101-131). backfillGate is a using var scoped to this foreach iteration, so when the switch below logs Abandoned and the iteration falls through to the next server, backfillGate.Dispose() runs immediately — while the abandoned slice is still executing against server.

The next tick's CollectQueryStoreAsync (RemoteCollectorService.QueryStore.cs) can then acquire the now-free gate for that same server and start live query_store collection while the wedged backfill slice is still running underneath it — the double-heavy-extraction scenario this PR exists to prevent, reintroduced in exactly the failure mode (a wedge) where it's most damaging.

The comment above ("an abandoned-but-still-wedged slice keeps the gate closed") describes the intended behavior, not the actual one. Releasing correctly needs the gate lease disposed off the true completion of work, not off RunAsync's return — e.g. a completion callback on AbandonableStep.RunAsync (today only onLateFault exists, and that fires solely on exceptions), or capturing work here and chaining the gate's Dispose() onto it instead of a using.

Identical bug in Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs around line 1922 — this isn't parity drift, both apps share the same flaw.

[claude]

Review summary

Reviewed the QueryStoreServerGate addition and its two wiring sites (Darling's DarlingWorker, Lite's RemoteCollectorService).

One correctness bug, present identically in both apps (left as inline comments):

The backfill side takes the gate with using var backfillGate = gate; outside the AbandonableStep, which is the right idea — but the using disposes the lease as soon as AbandonableStep.RunAsync returns, and on the Abandoned outcome it returns while the underlying task (work) is still running in the background (that's the whole point of "abandon, don't cancel" in AbandonableStep.cs). So a wedged slice past the 180s (Lite) / 300s (Darling) deadline releases the gate immediately, and the very next tick can start query_store collection against the same server while the abandoned slice is still executing underneath it — the exact concurrent-heavy-extraction scenario (measured ~128 MB in flight) this PR sets out to prevent, reintroduced specifically in the wedge case where it matters most. The doc comments and log lines both assert the opposite of what the code does.

• Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs:1922
• Lite/Services/RemoteCollectorService.QueryStoreBackfill.cs:99

This isn't Lite/Darling drift — both apps have the same flaw, from the same using-scoped-to-loop-iteration pattern. Fixing it needs the gate lease disposed off work's true completion (a new completion callback on AbandonableStep.RunAsync, since today only onLateFault exists and that's exception-only), not off RunAsync's synchronous return.

Everything else checked out:

• QueryStoreServerGate itself (PerformanceMonitor.Common/QueryStoreServerGate.cs): the interlocked CompareExchange/Lease/double-dispose-idempotence logic is correct, and matches what QueryStoreServerGateTests.cs pins.
• Tick-side wiring (Darling's RunOneAsync, Lite's CollectQueryStoreAsync) is a plain synchronous await with no detach/abandon semantics, so the gate there is held for the actual duration of the work — no equivalent issue.
• No security, injection, or secrets concerns — this is a pure in-process concurrency primitive, no new I/O or external input.
• No missing-index DMV suggestions (per instructions, not applicable to this diff anyway).

The new tests are solid for the primitive itself and for static wiring facts (same registry, gate-before-step ordering) but don't exercise the abandonment-then-release timing, which is how this slipped through.

[claude]

The gate does not actually stay held through an abandonment — it releases at the exact moment the comment says it must not.

The comment above (and the PR description) claims: "an abandoned-but-still-wedged slice keeps the gate closed... the tick must keep yielding while that statement is genuinely still running on the server."

But AbandonableStep.RunAsync returns Abandoned as soon as the deadline elapses — it does not wait for work to finish:

var finished = await Task.WhenAny(work, Task.Delay(timeout, cancellationToken));
if (finished != work)
{
    ...
    return new AbandonableStepResult(AbandonableStepOutcome.Abandoned);   // work is still running
}

work only truly completes later, in the .ContinueWith continuation that clears AbandonableStep's own _inFlight flag (PerformanceMonitor.Common/AbandonableStep.cs:108-130).

using var backfillGate = gate; is scoped to this loop iteration, so it Dispose()s as soon as RunAsync returns — i.e. immediately on Abandoned, not when the wedged statement actually finishes. Concretely:

1. A slice wedges past BackfillSliceDeadline (180s) → outcome is Abandoned, loop iteration ends, backfillGate.Dispose() runs — QueryStoreServerGate._taken goes back to 0 — while the abandoned task is still executing the QS extraction query against the monitored server.
2. On the next tick (~1 min later), RunOneAsync's IsQueryStoreCollector gate calls TryAcquire() on the same per-server gate and succeeds, because it was already released.
3. The tick's query_store collection now runs concurrently with the still-wedged backfill statement — the exact double-QS-extraction collision ([PERF] Tick QS collection and QS backfill run concurrently against the same server — serialize per server #2165) this PR exists to prevent, and it happens specifically in the case (a genuinely stuck/slow statement) where the collision is worst.

Note _backfillSliceSteps' own IsInFlight guard correctly stays set until work truly ends (that's the whole point of AbandonableStep) — but nothing propagates that same "still running" fact to QueryStoreServerGate, so the new gate doesn't inherit the protection the old guard already had.

The wiring tests (Darling_BothLoopsShareOneGateRegistry / Lite_BothLoopsShareOneGateRegistry in Lite.Tests/QueryStoreServerGateTests.cs) only assert that the gate acquisition line appears before the step textually — they don't (and structurally can't, being text-pin tests) catch that the lease is released too early on the abandoned path. Same issue exists in Lite/Services/RemoteCollectorService.QueryStoreBackfill.cs (the using var backfillGate = gate; there has the identical shape) — flagging both for parity.

A fix likely needs the gate's Dispose() to ride the same completion signal as AbandonableStep's _inFlight clear — e.g. transfer the lease into the onLateFault/continuation path (or a new completion hook on AbandonableStep) instead of disposing it when RunAsync merely returns control to the loop.

[claude]

Same premature-release bug as the Darling twin (DarlingWorker.cs around RunQueryStoreBackfillLoopAsync's using var backfillGate = gate;) — flagging here for parity since the same fix will need to land in both.

step.RunAsync (PerformanceMonitor.Common/AbandonableStep.cs) returns AbandonableStepOutcome.Abandoned the moment the deadline elapses, without awaiting the underlying work task — work keeps running and only clears AbandonableStep's own _inFlight guard later, from a .ContinueWith continuation.

using var backfillGate = gate; is scoped to this foreach iteration, so it disposes as soon as RunAsync returns — including on Abandoned, while the wedged QS extraction statement is still actually executing against the server. That means: slice wedges past BackfillSliceDeadline → gate released this iteration → the next CollectQueryStoreAsync tick (RemoteCollectorService.QueryStore.cs) successfully TryAcquire()s the same per-server gate and runs concurrently with the still-running abandoned backfill statement — the exact collision #2165 is meant to prevent, in precisely the "genuinely stuck" case that matters most.

The comment directly above this line ("an abandoned-but-still-wedged slice keeps the gate closed... the tick must keep yielding") states the intended behavior, but the code doesn't achieve it. Worth fixing in QueryStoreServerGate/the call sites so the lease's release rides the same completion signal AbandonableStep._inFlight uses, rather than RunAsync's early return on abandonment.

[claude]

Reviewed the diff (CHANGELOG, Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs, Lite/Services/RemoteCollectorService.QueryStore*.cs, PerformanceMonitor.Common/QueryStoreServerGate.cs, Lite.Tests/QueryStoreServerGateTests.cs).

One correctness finding, posted inline on both apps (parity bug — same root cause, same shape in each): the gate does not actually stay held through an Abandoned outcome, despite the comments and PR description explicitly claiming it does. AbandonableStep.RunAsync returns Abandoned the instant the deadline elapses, without waiting for the underlying task to finish; using var backfillGate = gate; is scoped to the loop iteration, so it disposes right then — releasing the per-server gate while the wedged Query Store statement is still actually running on the monitored server. The very next tick can then acquire the gate and run concurrently with that still-executing statement, which is the exact overlap #2165 sets out to prevent, and it happens specifically in the "genuinely stuck" case that matters most. The new wiring tests (Darling_BothLoopsShareOneGateRegistry / Lite_BothLoopsShareOneGateRegistry) only pin call-site ordering textually, so they pass despite this. Details and a suggested fix direction (ride AbandonableStep's own completion signal instead of RunAsync's early return) are in the two inline comments.

Everything else looks solid:

• QueryStoreServerGate itself (the interlocked flag, idempotent double-dispose via Interlocked.Exchange(ref _gate, null), the NotGated sentinel) is correct and well-tested for the properties it does cover (exclusion, non-blocking, per-server independence).
• Darling and Lite wire the gate at the described funnel points (RunOneAsync / CollectQueryStoreAsync), keyed consistently with each app's existing registries (ServerId int for Darling, server.Id string for Lite) — no key-type mismatch, no drift between the two apps' collector-name matching (OrdinalIgnoreCase against QueryStoreCollector.Instance.Name, consistent with s_dispatch's own comparer).
• No security-relevant surface here (no new input handling, no SQL text, pure in-process concurrency primitive).
• No missing-index DMV stuff, not applicable to this diff.

No other Lite/Darling parity drift spotted beyond the shared bug above (which itself is present identically in both, so no divergence between the two apps).