Decide, per site, whether the repair pass can be abandoned at the lock (#2465)

[erikdarlingdata]

Closes #2465. Lite only — Darling has no QueryStoreSliceRepairService and no lock of this shape, so there is no twin to keep in step.

The twenty are five

20 warning lines, 5 distinct call sites. Each is reported four times: MSBuild builds the WPF project twice (the real PerformanceMonitorLite.csproj and its generated _wpftmp pass) and each emits the warning in the log and again in the summary block. All five are AcquireReadLock(); the two AcquireWriteLock() acquisitions are invisible to CA2016 because that method has no token-taking overload.

Why the analyzer is right rather than pedantic

Every method in this service takes the database lock before it opens its connection. So a token that reaches the reads and not the lock is a token that stops at the door — the exact hole #2454 found on the analysis pass, where threading the reads alone would have produced a pass abandonable everywhere except where it was actually stuck. A startup repair queued behind a long archival has the same shape.

It is also worth saying plainly that nothing observable separates the two spellings today: MainWindow.xaml.cs:193 fires this un-awaited with no token, so cancellationToken is default at all five sites and CanBeCanceled is false, which takes AcquireReadLock's original uninterruptible path either way. That is the reason these need deciding and pinning rather than a reason they do not matter — the decision is entirely in what the code says, and a silent no-arg call is indistinguishable from an oversight.

The five, decided one at a time

The split is by what abandoning costs, not by whether the site reads or writes.

Site	Choice	Why
AlreadyRepairedAsync	forward	Asks a question, writes nothing. No reason to sit in an uninterruptible EnterReadLock() for an answer the caller has stopped wanting.
SurveyAsync	forward	The longest read here — a full GROUP BY over query_store_stats plus a read_parquet of every monthly archive file. This is the one that should yield to an archival rather than queue behind it.
RewriteArchiveFileAsync (rewrite-to-temp)	forward	The expensive phase, and abandoning it lands in a state the design already handles: the original is untouched by construction, the marker is withheld, and the next launch retries a repair the pre-fix signature makes idempotent. A cancelled COPY strands the same .repair-tmp sibling a failed one does, the retry's COPY overwrites it, and the archive glob does not match that suffix so a stranded one is never read as an archive file.
RepairOnStartupAsync (no-work marker)	None, with the reason	The marker's whole job is the sentence already above it in the source: "Record it so the survey does not run on every launch forever." Dropping it makes the survey run again — the expensive thing this service has.
RepairOnStartupAsync (post-repair marker)	None, with the reason	The repair is done — hot table collapsed and committed, every affected archive file rewritten and swapped — and abandoning the record undoes none of it. It only makes the next launch pay the full survey to rediscover there is nothing left to repair.

3 forwarded, 2 CancellationToken.None with a comment saying why, matching the convention #2454 set for its fourteen off-pass callers.

A detail that decided both declines: reaching either marker write proves the token was unfired a moment earlier, because SurveyAsync (or the entire RepairAsync) just returned under it. Forwarding there would buy an abandonment window microseconds wide, and what it would risk in that window is the record of work that cannot be redone for free.

The declines go all the way through

CancellationToken.None reaches the lock, the OpenAsync and both statements — not just the lock. #2454's InsertFindingsAsync declines only past the lock and the open, and it is right to: what it must not cut is a multi-row batch that would read as a complete analysis that found fewer problems. The marker write has no such shape — it is CREATE TABLE IF NOT EXISTS plus one row, and a cut between them leaves an empty marker table that AlreadyRepairedAsync correctly reads as "not repaired". What it must not lose is the record, so the decline covers the whole write. A lock that will not be abandoned in front of a write that will be is worse than either choice made whole.

Not suppressed, and the write locks are not swept in

No #pragma, no .editorconfig entry, no file-scoped suppression — per the issue, that converts a list of decidable items into a permanent unknown.

The two AcquireWriteLock() sites are left exactly as they are, and not merely because CA2016 cannot see them. They are the genuinely maintenance-shaped acquisitions — the hot-table mutation with its CHECKPOINT, and the instant a live path's bytes are replaced — which is the one category #2463 explicitly does not put in question. Their no-timeout behaviour is #2463's, and #2463 rules on it.

While in the file: the two marker writes are writes taking a read lock, which is the same shape #2455/#2464 ruled correct. Nothing about their locking changed here.

Verification

• The build is the check. Full-solution rebuild on macOS (EnableWindowsTargeting, -t:Rebuild so the analyzer actually re-runs — an incremental build reports zero because it skips the compile):

  	CA2016 lines	solution warning summary	errors
  dev	20	30	0
  this branch	0	20	0

  Warning code sets are otherwise identical (CA1720 ×8, xUnit1031 ×2, xUnit2031 ×30 on both). One new warning was introduced and fixed before commit: the first draft of the pin used Assert.Equal(0, …Count) and drew xUnit2013.

• Lite.Tests is net10.0-windows and cannot run on macOS, so the real Lite.Tests/QueryStoreSliceRepairTests.cs was compiled into a net10.0 xUnit shim together with the real shipped sources — Lite/Database/*.cs, Lite/Services/QueryStoreSliceRepairService.cs, ArchiveService.cs, ParquetCompaction.cs, the real SharedDuckDbFixture, and a ProjectReference to PerformanceMonitor.Collectors — with one harness-only stub for the single service-layer symbol DuckDbInitializer reaches for (RemoteCollectorService.GetDeterministicHashCode, a pure hash on a legacy re-key path nothing under test calls). That is the same single stub Hand Lite's analysis token to the store reads it is meant to abandon (#2443) #2454's Lite harness needed.

  10/10 pass on this branch. 2/10 fail on dev, against a real DuckDB:

  EveryLockAcquisition_SaysWhetherItCanBeAbandoned [FAIL]
    Assert.Empty() Failure: Collection was not empty
    Collection: [_duckDb.AcquireReadLock(), _duckDb.AcquireReadLock(), _duckDb.AcquireReadLock(),
                 _duckDb.AcquireReadLock(), _duckDb.AcquireReadLock()]
  
  EveryMutatingPhase_SitsInsideAWriteLockScope [FAIL]
    Not found: "using (_duckDb.AcquireReadLock(cancellationToken))"
  

  The second is the pre-existing structural pin, whose anchor for the rewrite-to-temp lock moved with the token — updated deliberately, not incidentally, and it still asserts the same property.

• Rebased onto dev at 9d1337a5 and every check above re-run after: still 0 CA2016, still 10/10 here and 2/10 on dev.

• CI is the arbiter for the rest of the suite.

The new test

EveryLockAcquisition_SaysWhetherItCanBeAbandoned sits beside the existing EveryMutatingPhase_SitsInsideAWriteLockScope in the same class, deliberately: one pins which lock each phase takes, the other pins whether the phase can be abandoned while it waits for it. Two properties, and splitting them across files would let either be reverted without the other noticing.

It asserts zero silent acquisitions, three forwards, two declines, that each decline carries #2465 and the word marker in the preceding source, that each decline is whole (the OpenAsync and both statements decline too), and that the write-lock count is still two.

Co-Authored-By: Claude Opus 5 noreply@anthropic.com

---

Related: #2469 answers #2463, and agrees with the call made here — the two AcquireWriteLock sites in this file are maintenance-shaped (clause 1 of the rule it states), and the two marker writes taking a read lock are appends, which is clause-correct too. Neither PR touches the other's files; they can merge in either order.

[claude]

Reviewed. This is a tightly-scoped, well-documented change and I didn't find correctness bugs.

Verified:

• The Lite/Darling parity claim in the description holds — Darling/PerformanceMonitor.Darling.Storage/QueryStoreSliceRepair.cs uses per-statement Npgsql timeouts, not a ReaderWriterLockSlim-style app lock, so there's genuinely no twin lock-forwarding decision to make there.
• The 3 forward / 2 decline split is internally consistent: AlreadyRepairedAsync, SurveyAsync, and the rewrite-to-temp phase of RewriteArchiveFileAsync forward cancellationToken; both marker writes in RepairOnStartupAsync go all the way through (lock, OpenAsync, and MarkRepairedAsync) on CancellationToken.None, matching the doc comment's stated rationale.
• The .repair-tmp stranding argument checks out — the archive glob is *_{Table}.parquet (QueryStoreSliceRepairService.cs:392), which doesn't match a path ending in .repair-tmp, so an abandoned temp sibling is never picked up as a real archive file on the next scan.
• The two untouched AcquireWriteLock() sites correctly have no token overload to forward to, so nothing was silently left inconsistent there.
• New test EveryLockAcquisition_SaysWhetherItCanBeAbandoned is a reasonable regex-based source guard for a decision (not a runtime behavior), and its counts/assertions match what's actually in the file (3 forwards, 2 declines, 2 OpenAsync(CancellationToken.None), 2 MarkRepairedAsync(..., CancellationToken.None), 2 AcquireWriteLock()).

No security, injection, or performance concerns — this only changes which token reaches an in-process ReaderWriterLockSlim wait, not any SQL construction. Nothing to flag inline.

[claude]

Reviewed. No correctness, parity, security, or performance issues found.

What I checked:

• Behavior for the 2 "declined" sites (RepairOnStartupAsync's marker writes): AcquireReadLock(CancellationToken.None) is behaviorally identical to the old no-arg AcquireReadLock() — the no-arg overload already delegates to CancellationToken.None (Lite/Database/DuckDbInitializer.cs:34), and CanBeCanceled is false either way, so this half of the change is a pure CA2016 lint fix with zero behavior change, as the PR body claims.
• Behavior for the 3 "forwarded" sites (AlreadyRepairedAsync, SurveyAsync, RewriteArchiveFileAsync's read-to-temp phase): forwarding the real token to the lock only changes anything when the token can actually fire, which today only happens under test — MainWindow.xaml.cs:193 fires this un-awaited with no token in production, matching the PR's own claim. Cancellation firing during the lock wait is safe in all three cases: no connection is open and nothing has been written yet at that point.
• RewriteArchiveFileAsync's .repair-tmp stranding: confirmed the archive glob (*_{Table}.parquet) doesn't match the .repair-tmp suffix, so a cancelled/failed COPY leaving a stranded temp file is correctly never picked up as an archive file, and the next attempt's COPY overwrites it. Pre-existing behavior, unaffected by this change.
• New test (EveryLockAcquisition_SaysWhetherItCanBeAbandoned): verified the regex counts (3 forwarded, 2 declined, 2 write locks untouched) against the actual source, and that the two declined-site "reason window" checks (1500 chars back) can't cross-contaminate since both marker-write comment blocks independently contain #2465 and marker.
• Lite/Darling parity: confirmed via grep that Darling's QueryStoreSliceRepair.cs uses a plain NpgsqlConnection + CancellationToken with no ReaderWriterLockSlim-style lock of any shape, so the PR body's claim that there's no Darling twin to keep in step holds up.
• CONTRIBUTING.md: XML doc comments on public APIs, PascalCase/_camelCase naming, and the "C# code compiles without warnings" checklist item are all satisfied — this PR is exactly what that last checklist item asks for.

Nice work tracing the cost/abandonability distinction per call site rather than blanket-suppressing or blanket-forwarding.