Repository navigation
Serialize the Query Store tick and backfill per server (#2165) #2272
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1913,6 +1913,22 @@ private async Task RunQueryStoreBackfillLoopAsync(QueryStoreBackfill backfill, L | |
| abandoned (loudly) and quarantined until its task actually dies, while every other | ||
| server's backfill continues. The deadline is a generous multiple of a healthy slice | ||
| (statement timeout 60s + store writes), so an abandonment is a defect signal. */ | ||
| /* #2165: the other half of the gate. Held for the WHOLE slice, and taken outside the | ||
| AbandonableStep so 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, which is exactly | ||
| the case the abandonment leaves behind. Zero-wait, so a tick already collecting simply defers | ||
| this server's slice to the next five-minute cycle. */ | ||
| var gate = _queryStoreGates.GetOrAdd(runtime.ServerId, static _ => new QueryStoreServerGate()).TryAcquire(); | ||
| if (gate is null) | ||
| { | ||
| _logger.LogInformation( | ||
| "query_store backfill slice on '{Server}' deferred — the tick's Query Store collection is running (#2165)", | ||
| runtime.Config.DisplayName); | ||
| continue; | ||
| } | ||
|
|
||
| using var backfillGate = gate; | ||
|
There was a problem hiding this comment. The gate is released as soon as
The very next tick can then acquire the now-free gate ( The doc comment right above ("an abandoned-but-still-wedged slice keeps the gate closed") and the
Same pattern, same bug, in the Lite twin:
Comment on lines
+1916
to
+1930
There was a problem hiding this comment. 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 var finished = await Task.WhenAny(work, Task.Delay(timeout, cancellationToken));
if (finished != work)
{
...
return new AbandonableStepResult(AbandonableStepOutcome.Abandoned); // work is still running
}
Note The wiring tests ( A fix likely needs the gate's |
||
|
|
||
| var step = _backfillSliceSteps.GetOrAdd(runtime.ServerId, static _ => new AbandonableStep()); | ||
| var result = await step.RunAsync( | ||
| () => backfill.RunServerSliceAsync(runtime, stoppingToken), | ||
|
|
@@ -1951,6 +1967,24 @@ private async Task RunQueryStoreBackfillLoopAsync(QueryStoreBackfill backfill, L | |
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// #2165: per-server gates shared by the tick's Query Store pass and the backfill slice, so the two never | ||
| /// run heavy QS text extraction against one server at the same time. Keyed by ServerId and never pruned, | ||
| /// like its <see cref="_backfillSliceSteps"/> sibling — one small object per server ever monitored. | ||
| /// | ||
| /// <para>Both loops must resolve the SAME gate instance for a server, which is what makes this one | ||
| /// dictionary rather than one per loop. Pinned by a test for that reason.</para> | ||
| /// </summary> | ||
| private readonly ConcurrentDictionary<int, QueryStoreServerGate> _queryStoreGates = new(); | ||
|
|
||
| /// <summary> | ||
| /// #2165: whether a dispatched collector name is the Query Store collector the gate covers. Compared | ||
| /// against the collector's OWN declared name rather than a literal, so renaming the collector cannot | ||
| /// silently unhook the gate and let the two loops overlap again. | ||
| /// </summary> | ||
| internal static bool IsQueryStoreCollector(string collectorName) => | ||
| string.Equals(collectorName, QueryStoreCollector.Instance.Name, StringComparison.OrdinalIgnoreCase); | ||
|
|
||
| /// <summary>#2148: per-server abandonment guards for the backfill loop — keyed by ServerId so a | ||
| /// removed-and-re-added server reuses its guard (harmless), and a wedged server never blocks its | ||
| /// neighbors. Never pruned: one small object per server ever monitored, bounded by fleet size.</summary> | ||
|
|
@@ -3921,6 +3955,26 @@ private async Task<int> RunOneAsync(ServerLoopState server, DarlingCollectorRunn | |
| return 0; | ||
| } | ||
|
|
||
| /* #2165: the tick's Query Store pass and the backfill slice both do heavy QS text extraction, and | ||
| they used to be free to run against the SAME server at once — measured as ~128 MB in flight on a | ||
| 4-core box, because a big catalog arriving triggers BOTH loops. Gated HERE because this is the one | ||
| funnel that has the runtime and the collector name together. Never waits: see QueryStoreServerGate | ||
| for why blocking a shared fleet loop would recreate the #2148 wedge through a lock. Skipping is safe | ||
| for this collector because its window is watermark-driven (#1960) — the next pass resumes from the | ||
| same boundary, so a skipped pass defers rows rather than dropping them. */ | ||
| using var queryStoreGate = IsQueryStoreCollector(collectorName) | ||
| ? _queryStoreGates.GetOrAdd(runtime.ServerId, static _ => new QueryStoreServerGate()).TryAcquire() | ||
| : QueryStoreServerGate.NotGated; | ||
|
|
||
| if (queryStoreGate is null) | ||
| { | ||
| _logger.LogInformation( | ||
| " [{Server}] query_store skipped this tick — its Query Store backfill slice is mid-flight (#2165). " + | ||
| "Resumes next tick from the same watermark; no rows are lost.", | ||
| server.Config.DisplayName); | ||
| return 0; | ||
| } | ||
|
|
||
| try | ||
| { | ||
| var result = await run(runner, runtime, cancellationToken); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theswitchbelow, regardless ofresult.Outcome. But perAbandonableStep.RunAsync(PerformanceMonitor.Common/AbandonableStep.cs:132-143),Abandonedmeans the underlying task (work) is still executing in the background; the step's own_inFlightguard is only cleared later, inwork.ContinueWithwhen the task truly ends.So on abandonment here:
backfillGatedisposes immediately, releasing the per-serverQueryStoreServerGate.RunOneAsync'sTryAcquire()on the same gate succeeds and launches a freshquery_storecollection 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_inFlightis cleared inwork.ContinueWith), not disposed based onRunAsyncmerely returning.Note the new
Darling_BothLoopsShareOneGateRegistrytest 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.