fix: serialize Azure DevOps client creation and hand callers a session [patch]

[matt-edmondson]

Fixes #287

What was wrong

EnsureAzureDevOpsClients read, disposed, nulled and reassigned the shared Connection / ProjectClient / BuildClient fields with no lock. BuildMonitor.UpdateAsync runs UpdateRepositoriesAsync / UpdateBuildsAsync / UpdateBuildAsync concurrently via Task.WhenAll across many owners and builds, and each independently calls it.

Two failure modes followed, both from re-reading those shared fields:

• Leaked connection. Two callers both find the fields stale, both Dispose()/null them, and both build a VssConnection. One is overwritten without ever being disposed.
• NullReferenceException. Every call site did EnsureAzureDevOpsClients(); if (BuildClient == null) return; and then dereferenced the field inside an async lambda that MakeAzureDevOpsRequestAsync only invokes after its semaphore and pacing delay. Another caller nulling the field in that window turned a checked-for "skipped, no client" path into an NRE.

What changed

New BuildMonitor/Providers/CredentialedSessionCache.cs — holds a connection-backed session, rebuilds it under a lock when the credentials change, disposes whatever it replaces, and returns the session as a value.

BuildMonitor/Providers/AzureDevOps.cs

• The connection and the two clients bound to it now live together in a nested AzureDevOpsSession, replaced as one value rather than three fields.
• EnsureAzureDevOpsClients() returns AzureDevOpsSession? instead of mutating fields, keeping the existing VssServiceException / UriFormatException handling and SetStatus behaviour.
• All five call sites take the session into a local and use session.ProjectClient / session.BuildClient, so nothing re-reads shared state across an await.
• AccountId / Token are snapshotted into locals once, rather than read separately for the staleness check and the connection.

The factory runs under the lock, so connection construction is serialized against another caller constructing one — that is the point, since the duplicate work and the leak came from exactly that overlap.

Acceptance criteria

Concurrent updates across multiple Azure DevOps owners, including right after a credential change, never throw NullReferenceException and never leak an undisposed VssConnection.

Both are asserted directly by the new tests.

Testing

BuildMonitor.Test/CredentialedSessionCacheTests.cs exercises the cache with a fake session. The concurrent scenarios release 32 callers through a Barrier and repeat 40 rounds, since one round proves nothing about a race.

Proven to fail without the fix: with the lock removed from Get (the original's state), 3 tests fail —

failed ConcurrentCallersOnAColdCacheShareOneSession
failed EverySupersededSessionIsDisposedExactlyOnce
failed ASessionHandedToACallerSurvivesARebuildBehindIt
  System.AggregateException: ... ---> System.NullReferenceException: Object reference not set to an instance of an object.

Both issue failure modes reproduced: duplicate/undisposed connections, and the NullReferenceException by name. With the lock restored, all 44 pass.

BuildMonitor.Test/AzureDevOpsSessionTests.cs covers the credential check — the branch deciding whether a connection is attempted at all, and the one part of the provider that runs offline.

Test run summary: Passed!  failed: 0  succeeded: 44

dotnet build BuildMonitor.sln: 0 warnings, 0 errors.

Known: the SonarCloud coverage gate

The gate wants 80% coverage on new code. Sonar measures 65 new lines with 36 uncovered (44.6%) — all 36 in AzureDevOps.cs; CredentialedSessionCache.cs, which holds the actual fix, is 29/29 covered. 4997dfa covers 5 more, so expect roughly 52% on re-analysis.

Of the rest, 13 lines build or call through a VssConnection and need a live organization and a real PAT — GetClient<T>() authenticates on the spot. The other 18 could be reached with a test seam, landing at about 80.0% with no margin.

The comment below has the measurement and three options, for the repo owner to choose between.

🤖 Generated with Claude Code

https://claude.ai/code/session_01SDNXPxBHkugDsgFMcgSdvP

[matt-edmondson]

SonarCloud quality gate: coverage on new code

Failing check: SonarCloud Code Analysis — 44.6% coverage on new code, gate requires ≥ 80%. Tests are green on all three platforms; this is the only red check.

Edited — my first version of this comment quoted 54.8% from a local measurement and said 80% was unreachable within this PR's scope. Both were wrong. The numbers below come from SonarCloud's own API, and there is a path to ~80%; it just has a cost worth stating plainly.

Where the new code sits

Sonar's own measure (new_lines_to_cover / new_uncovered_lines), taken on fd077d9:

File	To cover	Uncovered
Providers/CredentialedSessionCache.cs	29	0
Providers/AzureDevOps.cs	36	36
Total	65	36 → 44.6%

The synchronization fix — the part with the bug in it — is fully covered. The shortfall is entirely AzureDevOps.cs.

That reading predates 4997dfa, which added a test for the credential check. It covers 5 of those 36 (lines 20, 80–84), so once Sonar re-analyses, expect roughly 52%. Still short.

What the remaining 31 lines need

Three groups:

• 8 lines build the connection and clients (AzureDevOpsSession's constructor, CreateSession). VssConnection.GetClient<T>() authenticates against dev.azure.com on the spot — I probed it, and it throws VssUnauthorizedException: VS30063 offline. These need a live organization and a real PAT.
• 5 lines are the await session.ProjectClient/... calls themselves. Same.
• 18 lines are the five call-site guards plus the two catch returns. These are reachable without network, if session creation can be made to fail on demand.

Three options

1. Set SONAR_COVERAGE_EXCLUSIONS_EXTRA for this repo to cover **/Providers/**. The shared workflow documents this variable for exactly this case — "a file that cannot be executed rather than one nobody has got round to testing." It's a repo-settings change, so it isn't mine to make.

2. Make session creation injectable — an internal factory parameter defaulting to CreateSession, so a test can supply one that throws and drive the 18 reachable lines. That lands at roughly 80.0%: no margin at all, and it needs a workaround to set the credentials first, since BuildProviderAccountId doesn't round-trip through plain JsonSerializer without the app's converter. I didn't do this — it's a production design change made to move a metric 1.5 points, and one line of Sonar counting either way decides whether it works.

3. Abstract the Azure DevOps clients behind an interface, so the call sites can be driven with a fake. This is the version that actually makes the provider testable rather than gaming the threshold, and it comfortably clears the gate — but it's much larger than Unsynchronized Azure DevOps client (re)creation races under concurrent owner/build updates #287 asked for and belongs in its own PR.

My suggestion is (1) now, and (3) as a separate issue if the provider is worth making testable. I'd avoid (2). Happy to open that issue, or to do any of these here if you'd rather.

---

Generated by Claude Code

[sonarqubecloud]

Quality Gate failed

Failed conditions
52.3% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud