#2148: one wedged ladder step must never stop all collection

[erikdarlingdata]

Fix for the live 3.4.0 field regression (TrudAX, Azure elastic pool): all collection stopped permanently ~12 minutes after upgrading — every collector's last success frozen at the upgrade minute, last runs frozen shortly after, 40+ minutes of silence at screenshot time.

Root cause class

Lite's collection ladder runs its steps sequentially, and every step's exception armor was intact — but armor bounds throws, and nothing bounded a hang. Two steps could hold the loop without bound: the Query Store backfill tick (new in 3.4.0, and the prime suspect on the reporter's timeline — first contact after upgrade queues a backfill tail for every database in the pool) and the connection check (un-timeboxed Azure token/network paths). One wedge = every chart silent, permanently, with nothing in the log.

The fix

Both steps now run under AbandonableStep (new, in PerformanceMonitor.Common) — the ladder's own scheduled-analysis idiom extracted and made reusable: Task.WhenAny against a deadline + an in-flight guard cleared only when the task TRULY ends.

• Abandoned (deadline elapsed): the loop keeps collecting; ERROR logged naming [BUG] Regression on 3.4.0 - no CPU Data on Elastic Pool #2148.
• Quarantined: a wedged run is never overlapped by a relaunch (no stacking hung slices).
• Self-restoring: the moment the wedged task actually dies, the step runs again — a guard that never released would turn one hang into a permanently dead step, which is the bug again with extra steps.
• Deadlines (backfill 180s, connection check 90s) are generous multiples of healthy behavior (a tick is one 30s-capped slice per server), so an abandonment line is always a defect signal, never jitter.
• Shutdown reads as Cancelled, never a false ERROR.

Pins

All five outcomes; the quarantine (second run skipped while wedged, never overlapped); the release (step runs again when the wedge ends); abandoned-then-faulted is observed (no UnobservedTaskException) and still releases; synchronous delegate throws contained; cancellation semantics.

Notes

• This makes the failure survivable and diagnosable; the reporter's log (requested on the issue) will confirm which step wedged and whether the backfill needs its own deeper fix (e.g., off-ladder execution). Kept minimal deliberately — it's the safe 3.4.1 candidate.
• Flagging for Erik: this + #2143: retry drop_chunks once on deadlock before the DELETE fallback #2145 + the [BUG] PMLite 3.4.0 package installs binaries stamped 3.3.0 #2113 stamping fix look like a 3.4.1 patch release; the mis-stamped 3.4.0 assets ([BUG] When download lite 3.4.0, it still display 3.3.0 #2147) could be re-uploaded at the same time.

Closes nothing automatically — #2148 stays open pending the reporter's log confirmation.

[claude]

Review

Solid, well-tested fix for the core symptom (Lite's ladder permanently wedging on a hung backfill/connection-check step). AbandonableStep is a clean primitive and the pinned test suite covers the outcomes that matter (quarantine, release-on-completion, abandoned-then-faulted observed, cancellation-vs-abandonment). Two things worth addressing before this ships as the 3.4.1 candidate:

1. Lite/Darling parity gap: the identical bug still exists in Darling, unfixed

AbandonableStep lives in PerformanceMonitor.Common, shared by both apps, but this PR only wires it into Lite's CollectionBackgroundService. Darling's DarlingWorker.RunQueryStoreBackfillLoopAsync (Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs:1603-1649) has the exact same shape as the bug this PR fixes: it awaits backfill.RunServerSliceAsync(runtime, stoppingToken) for each server in a foreach, guarded only by a plain try/catch(Exception) with no deadline. A single server's slice wedging (the same "Azure elastic pool, big Query Store, first contact after upgrade" scenario from #2148) stalls this foreach forever, which stalls Query Store backfill for the entire fleet, silently, with no recovery — the same failure class, just with a foreach in place of Lite's straight-line sequence.

Notably, Darling's own scheduled-analysis pass already hand-rolls this exact Task.WhenAny(task, Task.Delay(...)) pattern elsewhere (which is presumably where "the ladder's own scheduled-analysis idiom" this PR extracts from actually originated) — so the fix is directly applicable here, just not applied. Worth a follow-up (or scope-widening this PR) to wrap RunServerSliceAsync in AbandonableStep too, otherwise the CHANGELOG's "one wedged step can no longer stop all collection" claim is only true for Lite.

2. Exceptions from an abandoned task that later actually faults are silently discarded

In AbandonableStep.RunAsync (PerformanceMonitor.Common/AbandonableStep.cs:92-101), the continuation attached to the abandoned work task only calls _ = t.Exception; to mark it observed (preventing UnobservedTaskException) — it never logs or otherwise surfaces that exception. Before this PR, every exception from the connection check / backfill tick was unconditionally logged via the direct try/catch. Now, if a step is abandoned (deadline elapsed) and the underlying task later actually throws, that exception is gone forever — no log line anywhere, just the generic "was ABANDONED" ERROR the caller already logged at abandonment time. Given the whole point of this PR is "survivable and diagnosable," it'd be worth at least logging the late fault (even without a logger reference in AbandonableStep itself, the caller could take an optional Action<Exception> callback, or CollectionBackgroundService could track a "late-fault" continuation per step for logging purposes).

Neither of these blocks the immediate 3.4.1 patch goal (which is explicitly scoped to Lite's field failure), but both should be tracked before calling the underlying bug closed.

[claude]

_ = t.Exception; only marks the exception observed to avoid UnobservedTaskException — it's never logged or surfaced anywhere. If a step is abandoned and its task later actually faults, that exception is now permanently lost: the caller already logged the generic "was ABANDONED" line at abandonment time and has no way to learn the eventual real failure reason. Given this PR's stated goal is "survivable and diagnosable," consider giving callers a way to observe the late fault (e.g. an optional Action<Exception> callback parameter, or exposing it via an event) rather than discarding it silently.

[erikdarlingdata]

Both findings taken in 6803474 — and both were right to insist on:

1. Darling parity: RunQueryStoreBackfillLoopAsync now wraps each server's slice in a per-SERVER AbandonableStep (keyed by ServerId, 300s deadline vs the 60s statement cap), so one wedged server is abandoned loudly and quarantined while the rest of the fleet keeps backfilling — strictly better than the Lite shape, where the whole tick shares one guard. The changelog claim is now true for both apps.

2. Late faults: RunAsync gains onLateFault, wired at all three call sites as 'this is the wedge's own exception ([BUG] Regression on 3.4.0 - no CPU Data on Elastic Pool #2148)'. It fires only for faults the awaited path did not already return, so ordinary failures never double-log; the microsecond race at the abandonment flag can cost the callback (never double it), which loses one log line, not correctness — documented on the parameter. A throwing callback still releases the guard, pinned.

Three new pins cover the callback arms.

[claude]

Correctness / Lite-Darling parity: this wraps the whole multi-server tick, not one server, so it doesn't actually give Lite the per-server isolation Darling gets.

RunQueryStoreBackfillTickAsync (Lite/Services/RemoteCollectorService.QueryStoreBackfill.cs:54) internally foreaches over every enabled server and runs one slice per server, unchanged by this PR:

public async Task RunQueryStoreBackfillTickAsync(CancellationToken cancellationToken)
{
    foreach (var server in _serverManager.GetEnabledServers())
    {
        ...
        await RunQueryStoreBackfillSliceAsync(server, cancellationToken); // still just try/catch, no per-slice timeout
    }
}

Here, _backfillStep is a single shared AbandonableStep instance wrapping that entire multi-server call, not one instance per server the way Darling does it (_backfillSliceSteps keyed by ServerId in DarlingWorker.cs, per the comment on that dictionary: "a wedged server never blocks its neighbors"). The comment on this call even cites the exact field failure ("elastic pool, big Query Store, first contact after upgrade") that this PR is supposed to fix.

Two consequences:

1. A hang in one server's slice still stalls the whole fleet's backfill — permanently. RunQueryStoreBackfillSliceAsync's hang isn't caught by the inner try/catch (it's a hang, not a throw), so the outer call wedges exactly like before. After 180s it gets reported Abandoned, but because _backfillStep is single/shared, its guard doesn't release until that whole wedged Task (stuck on server Add check for updates feature #1) truly finishes — which, if it's a genuine wedge, is never. Every subsequent due-check then returns SkippedStillRunning forever, so Query Store backfill silently dies for every server in the fleet, not just the wedged one, until the service restarts. That's a narrower version of the original [BUG] Regression on 3.4.0 - no CPU Data on Elastic Pool #2148 symptom, just scoped to backfill instead of all collection.
2. False-positive abandonment as fleet size grows, even with no wedge at all: the 180s deadline is sized for "one 30s-capped slice per server plus writes" (per the comment two lines up), but it's applied to the sum across all enabled servers in one tick. A healthy fleet of ~7+ servers, each taking close to the 30s cap, can legitimately blow the 180s budget and get spuriously Abandoned/SkippedStillRunning with no wedge involved.

Darling's fix (per-server ConcurrentDictionary<int, AbandonableStep> inside the foreach, keyed by ServerId) is the right shape and is even labeled "#2148 parity (review catch on the Lite fix)" — but that insight doesn't appear to have been carried back into Lite. Consider moving the AbandonableStep down into RunQueryStoreBackfillTickAsync's per-server loop (one step per server.ServerId, deadline sized to a single slice) rather than wrapping the whole tick here.

[claude]

Review summary

Reviewed the AbandonableStep primitive, its two Lite call sites, and its Darling backfill call site.

Main finding (left inline on Lite/Services/CollectionBackgroundService.cs): Lite's Query Store backfill fix wraps the entire multi-server RunQueryStoreBackfillTickAsync() call in one shared AbandonableStep, whereas Darling's equivalent fix (in DarlingWorker.cs) keys a ConcurrentDictionary<int, AbandonableStep> per server inside the per-server foreach. Since Lite's inner loop over _serverManager.GetEnabledServers() is untouched by this PR, a hang on one server's slice still isn't caught per-server — it trips the shared step's 180s deadline for the whole tick, and because the guard only releases when that whole wedged task finishes, backfill silently stalls for every server, forever, once one server wedges. That's a narrower version of the original #2148 symptom (now scoped to backfill instead of all collection) rather than the per-server isolation the PR title/description claims, and it's the opposite of what the sibling Darling code (explicitly commented "#2148 parity (review catch on the Lite fix)") does correctly. It also creates a false-positive-abandonment risk as fleet size grows, independent of any wedge, since the 180s deadline is sized for "one slice" but applied to the sum of all enabled servers' slices.

Secondary/minor: The PR frames AbandonableStep as extracting "the ladder's own scheduled-analysis idiom ... and making it reusable," but neither Lite's nor Darling's existing hand-rolled scheduled-analysis guard (CollectionBackgroundService.cs _analysisInFlight / DarlingWorker.cs RunAnalysisPassAsync) was migrated to use it. Not a bug, but worth a follow-up so there aren't three parallel copies of the same deadline+in-flight-guard pattern.

Things that look correct:

• AbandonableStep.RunAsync itself: the in-flight guard via CompareExchange, the "abandoned-then-faulted" observation path (avoids UnobservedTaskException, still fires onLateFault), synchronous-throw handling, and the documented (and reasonably accepted) narrow race where a fault landing in the few microseconds between the deadline firing and the abandoned flag being set can miss the onLateFault callback — that's explicitly called out in the XML doc as a "costs one log line, not correctness" tradeoff, which is a fair call.
• Cancellation semantics: shutdown correctly reads as Cancelled rather than a false ERROR, both in the primitive and at both call sites.
• Faulted vs Abandoned vs SkippedStillRunning logging matches the described severities (deadline/quarantine hits are ERROR, ordinary per-item failures stay WARNING).
• No SQL/T-SQL surface touched by this PR, so the style guide and missing-index-DMV concerns don't apply here — this is pure C#.

No security concerns (no new external input, file, network, or process surface — this is purely internal task-orchestration plumbing).

[erikdarlingdata]

Round-2 main finding taken in 3774ae5 — correct on both counts: the shared tick-level guard was a narrower rerun of the very symptom this PR fixes (one wedge → all Lite servers' backfill quarantined forever) AND a false-abandonment risk scaling with fleet size. Lite's guards now live inside the tick, per server, in exactly the Darling loop's shape; the ladder-level wrapper is gone because the tick is bounded by construction.

The secondary point (migrating both apps' hand-rolled scheduled-analysis guards onto AbandonableStep so there aren't three copies of the pattern) is real but out of scope for the 3.4.1 candidate — noting it on #2148 as follow-up.

[claude]

This entry is stale relative to where the PR ended up (commit 3, "Lite backfill guards go per-server, tick-level guard removed"):

• "The backfill tick ... now run[s] under an abandonment deadline with an in-flight guard" is no longer accurate. The tick-level AbandonableStep/BackfillTickDeadline from commit 1 was deliberately removed — RunQueryStoreBackfillIfDueAsync now wraps the whole tick in a plain try/catch (no deadline) because the guard moved inside the tick, per server (RemoteCollectorService.QueryStoreBackfill.cs, _backfillSliceSteps). A reader diagnosing a future wedge from this changelog would look for a tick-level abandonment log line that no longer exists.
• The entry only describes the Lite side. The identical failure class was also fixed in Darling (DarlingWorker.RunQueryStoreBackfillLoopAsync, per-server _backfillSliceSteps keyed by int ServerId, 300s deadline) in the same PR (commit 2), but that's not mentioned at all — other entries in this file (e.g. the PVS alert, the alert-threshold entry above) explicitly call out "both apps" or "Darling only" when a fix spans editions, so this looks like an oversight rather than a deliberate Lite-only framing.

Worth updating to say the guard lives per-server (inside the tick / inside the fleet loop) rather than on the tick itself, and to note the Darling-side fix.

[claude]

Review summary

Reviewed the AbandonableStep primitive and its wiring into Lite's CollectionBackgroundService/RemoteCollectorService.QueryStoreBackfill.cs and Darling's DarlingWorker.RunQueryStoreBackfillLoopAsync, plus the new tests. This is a well-reasoned fix and the PR's own history shows it already went through two rounds of self-review (Darling parity, late-fault surfacing, then the tick-level→per-server guard correction). I traced the final state of all four touched code files rather than just the diff, since the deadline sizing and guard placement changed across commits.

Correctness (PerformanceMonitor.Common/AbandonableStep.cs) — the guard lifecycle (Interlocked.CompareExchange acquire, ContinueWith release on true completion), the abandoned/late-fault race window, and the cancellation-vs-abandonment distinction all check out. The one race the code doesn't fully close (a fault landing in the microseconds between the deadline decision and abandoned.Value being set can miss onLateFault) is explicitly called out in the doc comment and accepted as "costs one log line, not correctness" — agreed, that's a reasonable tradeoff, not a bug.

Lite/Darling parity — confirmed correct on the specific thing that broke earlier in this PR's own history: Darling's _backfillSliceSteps is keyed by int ServerId (matches ServerRuntime.ServerId), Lite's is keyed by string with StringComparer.Ordinal (matches ServerConnection.Id, which really is a Guid-seeded string, not an int — commit 4's fix is correct). The per-server guard shape, deadline reasoning, and log vocabulary are consistent between the two apps. Lite's connection-check step has no Darling counterpart, but that's correct — CheckAllConnectionsAsync is a Lite-only concept (Darling has no equivalent fleet-wide blocking connectivity gate in its ladder).

One finding posted inline on CHANGELOG.md line 25: the entry describes the first-commit architecture (tick-level backfill guard) rather than where the PR ended up (per-server guard, tick-level guard explicitly removed in commit 3), and doesn't mention that the identical fix also landed in Darling. Worth fixing before this ships since the changelog is clearly meant to be the field-diagnosis reference for exactly this kind of incident.

No security, SQL-injection, or missing-index concerns apply here — this PR is pure C# task-orchestration, no query surface or user input touched.

[erikdarlingdata]

Round-3 finding taken in a2cbced: the changelog entry now describes the shipped architecture — per-server quarantine in both apps, late-fault surfacing, and the connection-check treatment — instead of the first-commit tick-level shape. Agreed on the reasoning: this entry is the field-diagnosis reference for exactly this incident class, so it describing a superseded design would have cost someone an hour.

[claude]

Reviewed the diff against `origin/dev` (6 files, +526/-34). Note: `CLAUDE.md` is gitignored in this repo and not present in the working tree, so I reviewed against `CONTRIBUTING.md`'s C# style section and the existing code conventions instead.

Summary: solid fix, no correctness bugs or parity gaps found.

• AbandonableStep.RunAsync (PerformanceMonitor.Common/AbandonableStep.cs) is correct on the paths that matter: the in-flight guard is released exactly once regardless of outcome (sync throw, fault, cancel, or eventual completion after abandonment), t.Exception is observed unconditionally in the ContinueWith so an abandoned-then-faulted task can never surface as an UnobservedTaskException, and a throwing onLateFault callback is swallowed so it can't leave the guard stuck. The one documented race (a fault landing in the few microseconds between the deadline firing and abandoned.Value being set to true) is called out honestly in the XML doc as "costs one log line, not correctness" — agreed, that's an acceptable trade-off, not a bug.
• Lite/Darling parity is good: both apps get per-server AbandonableStep guards (keyed by ConcurrentDictionary<TKey, AbandonableStep>, never pruned, static lambda factories to avoid closure allocations) around the Query Store backfill slice, with matching outcome-handling switches (Cancelled → return, Faulted w/ OperationCanceledException → return, Faulted → warn, Abandoned/SkippedStillRunning → error). Lite additionally wraps the fleet-wide CheckAllConnectionsAsync() call; Darling doesn't have an equivalent because its sweep loop already launches per-server bodies concurrently behind a semaphore gate (Darling: bounded-parallel fire-and-track collection sweep + cadence jitter (#1553) #1553 D2) rather than a shared sequential connection check — that's a pre-existing architectural difference, not a gap introduced by this PR.
• Test coverage (Lite.Tests/AbandonableStepTests.cs) exercises the outcomes that matter most: quarantine-while-wedged, guard release when the wedge finally ends, the abandoned-then-faulted double, the late-fault callback (including a throwing callback), and cancellation-vs-abandonment distinction. Good adversarial coverage.
• CHANGELOG.md entry and copyright headers (2026, matching other recently-touched files) follow existing conventions.

Nothing blocking. Nice writeup in the PR description tying this back to the specific field failure.