From fd077d93063a0e376d3599eec77e19dae23a117a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 21 Sep 2026 22:30:38 +0000 Subject: [PATCH 1/4] fix: serialize Azure DevOps client creation and hand callers a session [patch] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit EnsureAzureDevOpsClients read, disposed, nulled and reassigned the shared Connection/ProjectClient/BuildClient fields with no lock, while UpdateAsync runs UpdateRepositoriesAsync/UpdateBuildsAsync/UpdateBuildAsync concurrently over many owners and builds, each calling it independently. Two concurrent callers could both find the fields stale, both dispose and null them, and both build a VssConnection — leaving one connection overwritten and undisposed. Worse, a caller already past its own null check but delayed inside MakeAzureDevOpsRequestAsync's pacing delay dereferenced BuildClient after another caller had nulled it, which is a NullReferenceException rather than the "skipped, no client" path it checked for. Both followed from re-reading the shared fields. The connection and the two clients bound to it now live in an AzureDevOpsSession, cached by CredentialedSessionCache, which rebuilds under a lock when the credentials change and hands each caller the session as a value. Callers hold what they were given for the whole of their request, so a rebuild behind them cannot disturb a request in flight, and a superseded session is always disposed. Fixes #287 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SDNXPxBHkugDsgFMcgSdvP --- .../CredentialedSessionCacheTests.cs | 276 ++++++++++++++++++ BuildMonitor/Providers/AzureDevOps.cs | 131 +++++---- .../Providers/CredentialedSessionCache.cs | 117 ++++++++ 3 files changed, 474 insertions(+), 50 deletions(-) create mode 100644 BuildMonitor.Test/CredentialedSessionCacheTests.cs create mode 100644 BuildMonitor/Providers/CredentialedSessionCache.cs diff --git a/BuildMonitor.Test/CredentialedSessionCacheTests.cs b/BuildMonitor.Test/CredentialedSessionCacheTests.cs new file mode 100644 index 0000000..06be3b8 --- /dev/null +++ b/BuildMonitor.Test/CredentialedSessionCacheTests.cs @@ -0,0 +1,276 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.BuildMonitor.Test; + +using System.Collections.Concurrent; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +/// +/// Tests for the synchronization half of the Azure DevOps client lifecycle. +/// +/// +/// AzureDevOps.EnsureAzureDevOpsClients cannot be driven from a test: building a session +/// constructs a real VssConnection against dev.azure.com, and the update methods around it +/// need an ImGui application and live credentials. The part that went wrong is not any of that — +/// it is the caching and the handover, which holds +/// on its own and which a fake session exercises exactly. +/// +[TestClass] +public sealed class CredentialedSessionCacheTests +{ + /// + /// Enough concurrent callers to reliably overlap. UpdateAsync fans out over every owner, + /// repository, build and run at once, so this is not an unrealistic number of them. + /// + private const int ConcurrentCallers = 32; + + /// + /// Repeats of each concurrent scenario. A race is probabilistic, so one round proves nothing; + /// repeating narrows the chance of an unsynchronized cache slipping through to negligible. + /// + private const int Rounds = 40; + + private const string AccountId = "contoso"; + private const string Token = "pat-1"; + private const string RotatedToken = "pat-2"; + + /// + /// Stands in for the connection and clients an Azure DevOps session owns. The clients are what a + /// caller dereferences after its rate-limit delay, so the fake tracks whether it is still usable + /// at that point. + /// + private sealed class FakeSession : IDisposable + { + private int disposals; + + internal int Disposals => Volatile.Read(ref disposals); + + internal bool IsDisposed => Disposals > 0; + + internal string Credentials { get; } + + internal FakeSession(string credentials) => Credentials = credentials; + + public void Dispose() => Interlocked.Increment(ref disposals); + } + + /// + /// Builds a cache that records every session it hands out, widening the creation window so an + /// unsynchronized cache has a fair chance to interleave rather than passing by luck. + /// + private static CredentialedSessionCache CreateCache(ConcurrentBag created) => + new((accountId, token) => + { + Thread.Yield(); + FakeSession session = new($"{accountId}:{token}"); + created.Add(session); + return session; + }); + + /// + /// Releases every caller at once and collects what each one got back. + /// + private static List RunConcurrently(Func callerBody) + { + ConcurrentBag results = []; + using Barrier barrier = new(ConcurrentCallers); + + Parallel.For(0, ConcurrentCallers, index => + { + barrier.SignalAndWait(); + results.Add(callerBody(index)); + }); + + return [.. results]; + } + + /// + /// The leak half of the race. Callers arriving together on a cold cache must agree on one + /// session: unsynchronized, several of them each see no session, each build one, and every + /// connection but the last is overwritten without being disposed. + /// + [TestMethod] + public void ConcurrentCallersOnAColdCacheShareOneSession() + { + for (int round = 0; round < Rounds; round++) + { + ConcurrentBag created = []; + using CredentialedSessionCache cache = CreateCache(created); + + List handedOut = RunConcurrently(_ => cache.Get(AccountId, Token)); + + Assert.AreEqual(1, cache.SessionsCreated, $"round {round}: more than one session was built"); + Assert.AreEqual(1, created.Count, $"round {round}: more than one session was built"); + Assert.AreEqual(ConcurrentCallers, handedOut.Count); + Assert.IsTrue( + handedOut.TrueForAll(session => ReferenceEquals(session, handedOut[0])), + $"round {round}: callers were handed different sessions"); + } + } + + /// + /// No session may be dropped without being disposed, which is the leaked VssConnection the + /// issue names. Callers rotate the token underneath each other here, so sessions really are being + /// replaced while others are asking for one. + /// + [TestMethod] + public void EverySupersededSessionIsDisposedExactlyOnce() + { + for (int round = 0; round < Rounds; round++) + { + ConcurrentBag created = []; + CredentialedSessionCache cache = CreateCache(created); + + List handedOut = RunConcurrently( + index => cache.Get(AccountId, index % 2 == 0 ? Token : RotatedToken)); + + cache.Dispose(); + + Assert.AreEqual(created.Count, cache.SessionsCreated); + foreach (FakeSession session in created) + { + Assert.AreEqual(1, session.Disposals, $"round {round}: a session was disposed {session.Disposals} times"); + } + + Assert.AreEqual(ConcurrentCallers, handedOut.Count); + } + } + + /// + /// The half. A caller is handed its session before waiting + /// out the rate-limit delay and dereferences the clients afterwards. A rebuild behind it used to + /// null the shared fields in that window; holding the session as a value means the caller still + /// has what it checked. + /// + [TestMethod] + public void ASessionHandedToACallerSurvivesARebuildBehindIt() + { + for (int round = 0; round < Rounds; round++) + { + ConcurrentBag created = []; + using CredentialedSessionCache cache = CreateCache(created); + + List observed = RunConcurrently(index => + { + // Half the callers rotate the credentials, standing in for a PAT change or a second + // configured owner; the rest take a session and use it after a pause. + string token = index % 2 == 0 ? Token : RotatedToken; + FakeSession session = cache.Get(AccountId, token); + Thread.Yield(); + return session.Credentials; + }); + + Assert.AreEqual(ConcurrentCallers, observed.Count); + Assert.IsFalse( + observed.Exists(string.IsNullOrEmpty), + $"round {round}: a caller dereferenced a session it no longer held"); + } + } + + /// + /// Credentials that have not changed must not cause a rebuild, since that is the whole reason the + /// session is cached rather than built per request. + /// + [TestMethod] + public void UnchangedCredentialsReuseTheCachedSession() + { + ConcurrentBag created = []; + using CredentialedSessionCache cache = CreateCache(created); + + FakeSession first = cache.Get(AccountId, Token); + FakeSession second = cache.Get(AccountId, Token); + + Assert.AreSame(first, second); + Assert.AreEqual(1, cache.SessionsCreated); + Assert.IsFalse(first.IsDisposed); + } + + /// + /// A changed token rebuilds, and the session it replaces is disposed rather than dropped. + /// + [TestMethod] + public void ChangedCredentialsRebuildAndDisposeThePreviousSession() + { + ConcurrentBag created = []; + using CredentialedSessionCache cache = CreateCache(created); + + FakeSession first = cache.Get(AccountId, Token); + FakeSession second = cache.Get(AccountId, RotatedToken); + + Assert.AreNotSame(first, second); + Assert.AreEqual(2, cache.SessionsCreated); + Assert.IsTrue(first.IsDisposed); + Assert.IsFalse(second.IsDisposed); + } + + /// + /// A factory that throws — an unreachable organization, a malformed account name — must not leave + /// the credentials recorded, or the cache would answer the next caller with a session it never + /// built. + /// + [TestMethod] + public void AFailedBuildLeavesNothingCachedForThoseCredentials() + { + int attempts = 0; + using CredentialedSessionCache cache = new((accountId, token) => + ++attempts == 1 + ? throw new InvalidOperationException("connection refused") + : new FakeSession($"{accountId}:{token}")); + + _ = Assert.ThrowsExactly(() => cache.Get(AccountId, Token)); + + FakeSession recovered = cache.Get(AccountId, Token); + + Assert.AreEqual(2, attempts); + Assert.AreEqual($"{AccountId}:{Token}", recovered.Credentials); + } + + /// + /// Invalidation disposes what is cached and forces the next caller to build, which is what a + /// caller does after an authentication failure. + /// + [TestMethod] + public void InvalidateDisposesTheSessionAndForcesARebuild() + { + ConcurrentBag created = []; + using CredentialedSessionCache cache = CreateCache(created); + + FakeSession first = cache.Get(AccountId, Token); + cache.Invalidate(); + FakeSession second = cache.Get(AccountId, Token); + + Assert.IsTrue(first.IsDisposed); + Assert.AreNotSame(first, second); + Assert.AreEqual(2, cache.SessionsCreated); + } + + /// + /// Disposing the cache disposes the connection it holds, and is safe to repeat. + /// + [TestMethod] + public void DisposeDisposesTheCachedSessionOnce() + { + ConcurrentBag created = []; + CredentialedSessionCache cache = CreateCache(created); + FakeSession session = cache.Get(AccountId, Token); + + cache.Dispose(); + cache.Dispose(); + + Assert.AreEqual(1, session.Disposals); + } + + /// + /// Asking a disposed cache for a session is a programming error rather than a silently missing + /// client. + /// + [TestMethod] + public void GetAfterDisposeThrows() + { + ConcurrentBag created = []; + CredentialedSessionCache cache = CreateCache(created); + cache.Dispose(); + + _ = Assert.ThrowsExactly(() => cache.Get(AccountId, Token)); + } +} diff --git a/BuildMonitor/Providers/AzureDevOps.cs b/BuildMonitor/Providers/AzureDevOps.cs index 67f2ffa..885ad17 100644 --- a/BuildMonitor/Providers/AzureDevOps.cs +++ b/BuildMonitor/Providers/AzureDevOps.cs @@ -17,55 +17,86 @@ internal sealed class AzureDevOps : BuildProvider internal static BuildProviderName BuildProviderName => nameof(AzureDevOps).As(); internal override BuildProviderName Name => BuildProviderName; - private VssConnection? Connection { get; set; } - private ProjectHttpClient? ProjectClient { get; set; } - private BuildHttpClient? BuildClient { get; set; } - private string? LastAccountId { get; set; } - private string? LastToken { get; set; } + private CredentialedSessionCache Sessions { get; } = new(CreateSession); private bool ShouldDiscoverProjects { get; set; } - private void EnsureAzureDevOpsClients() + /// + /// A connection to an Azure DevOps organization together with the clients built from it. + /// + /// + /// The clients are bound to the connection that made them, so they are kept and replaced as one + /// value. A caller holds the session it was handed for the whole of its request, which is what + /// keeps a rebuild behind it from nulling a client it is about to use. + /// + internal sealed class AzureDevOpsSession : IDisposable { - if (string.IsNullOrEmpty(AccountId) || string.IsNullOrEmpty(Token)) + private VssConnection Connection { get; } + + /// + /// Gets the client used to enumerate the organization's projects. + /// + internal ProjectHttpClient ProjectClient { get; } + + /// + /// Gets the client used to read build definitions, builds and runs. + /// + internal BuildHttpClient BuildClient { get; } + + /// + /// Initializes a new instance of the class. + /// + /// The connection the session owns and disposes. + internal AzureDevOpsSession(VssConnection connection) { - return; + Connection = connection; + ProjectClient = connection.GetClient(); + BuildClient = connection.GetClient(); } - // Only recreate when credentials have changed - if (Connection != null && LastAccountId == AccountId.ToString() && LastToken == Token.ToString()) + /// + /// Disposes the underlying connection. + /// + public void Dispose() => Connection.Dispose(); + } + + private static AzureDevOpsSession CreateSession(string accountId, string token) + { + Uri collectionUri = new($"https://dev.azure.com/{accountId}"); + VssBasicCredential credentials = new(string.Empty, token); + return new(new VssConnection(collectionUri, credentials)); + } + + /// + /// Gets the session for the current credentials, or when there are none or + /// the connection could not be built. + /// + /// + /// Callers keep the returned session in a local rather than reading it again. Re-reading is what + /// let a rebuild on another thread null a client between a caller's null check and its use. + /// + /// The session, or . + private AzureDevOpsSession? EnsureAzureDevOpsClients() + { + string accountId = AccountId.ToString(); + string token = Token.ToString(); + if (string.IsNullOrEmpty(accountId) || string.IsNullOrEmpty(token)) { - return; + return null; } - // Dispose old connection before creating a new one - Connection?.Dispose(); - Connection = null; - ProjectClient = null; - BuildClient = null; - try { - Uri collectionUri = new($"https://dev.azure.com/{AccountId}"); - VssBasicCredential credentials = new(string.Empty, Token); - Connection = new(collectionUri, credentials); - ProjectClient = Connection.GetClient(); - BuildClient = Connection.GetClient(); - LastAccountId = AccountId.ToString(); - LastToken = Token.ToString(); + return Sessions.Get(accountId, token); } catch (VssServiceException ex) { - Connection = null; - ProjectClient = null; - BuildClient = null; SetStatus(ProviderStatus.Error, $"{Strings.ConnectionErrorMessage} {ex.Message}"); + return null; } catch (UriFormatException ex) { - Connection = null; - ProjectClient = null; - BuildClient = null; SetStatus(ProviderStatus.Error, $"Invalid organization name: {ex.Message}"); + return null; } } @@ -118,17 +149,17 @@ internal async Task DiscoverProjectsAsync() return; } - EnsureAzureDevOpsClients(); - if (ProjectClient == null) + AzureDevOpsSession? session = EnsureAzureDevOpsClients(); + if (session == null) { - Log.Warning($"{Name}: DiscoverProjectsAsync skipped - ProjectClient is null after credential update"); + Log.Warning($"{Name}: DiscoverProjectsAsync skipped - no Azure DevOps session after credential update"); return; } Log.Info($"{Name}: Discovering projects for organization '{AccountId}'"); await MakeAzureDevOpsRequestAsync($"{Name}/discover", async () => { - IEnumerable projects = await ProjectClient.GetProjects().ConfigureAwait(false); + IEnumerable projects = await session.ProjectClient.GetProjects().ConfigureAwait(false); int projectCount = 0; foreach (TeamProjectReference? project in projects) @@ -159,10 +190,10 @@ internal override async Task UpdateRepositoriesAsync(Owner owner) return; } - EnsureAzureDevOpsClients(); - if (ProjectClient == null) + AzureDevOpsSession? session = EnsureAzureDevOpsClients(); + if (session == null) { - Log.Warning($"{Name}: UpdateRepositoriesAsync skipped for owner '{owner.Name}' - ProjectClient is null"); + Log.Warning($"{Name}: UpdateRepositoriesAsync skipped for owner '{owner.Name}' - no Azure DevOps session"); return; } @@ -171,7 +202,7 @@ await MakeAzureDevOpsRequestAsync($"{Name}/{owner.Name}", async () => { // In Azure DevOps, projects are the top-level containers // The owner name represents a project in Azure DevOps - IEnumerable projects = await ProjectClient.GetProjects().ConfigureAwait(false); + IEnumerable projects = await session.ProjectClient.GetProjects().ConfigureAwait(false); bool foundProject = false; int projectCount = 0; @@ -216,17 +247,17 @@ internal override async Task UpdateBuildsAsync(Repository repository) return; } - EnsureAzureDevOpsClients(); - if (BuildClient == null) + AzureDevOpsSession? session = EnsureAzureDevOpsClients(); + if (session == null) { - Log.Warning($"{Name}: UpdateBuildsAsync skipped for '{repository.Owner.Name}/{repository.Name}' - BuildClient is null"); + Log.Warning($"{Name}: UpdateBuildsAsync skipped for '{repository.Owner.Name}/{repository.Name}' - no Azure DevOps session"); return; } Log.Debug($"{Name}: UpdateBuildsAsync for '{repository.Owner.Name}/{repository.Name}'"); await MakeAzureDevOpsRequestAsync($"{Name}/{repository.Owner.Name}/{repository.Name}", async () => { - List definitions = await BuildClient.GetDefinitionsAsync(repository.Owner.Name).ConfigureAwait(false); + List definitions = await session.BuildClient.GetDefinitionsAsync(repository.Owner.Name).ConfigureAwait(false); Log.Info($"{Name}: Found {definitions.Count} build definition(s) for '{repository.Owner.Name}/{repository.Name}'"); foreach (BuildDefinitionReference? definition in definitions) { @@ -270,17 +301,17 @@ internal override async Task UpdateBuildAsync(Build build) return; } - EnsureAzureDevOpsClients(); - if (BuildClient == null) + AzureDevOpsSession? session = EnsureAzureDevOpsClients(); + if (session == null) { - Log.Warning($"{Name}: UpdateBuildAsync skipped for '{build.Owner.Name}/{build.Repository.Name}/{build.Name}' - BuildClient is null"); + Log.Warning($"{Name}: UpdateBuildAsync skipped for '{build.Owner.Name}/{build.Repository.Name}/{build.Name}' - no Azure DevOps session"); return; } Log.Debug($"{Name}: UpdateBuildAsync for '{build.Owner.Name}/{build.Repository.Name}/{build.Name}' (definition ID: {build.Id})"); await MakeAzureDevOpsRequestAsync($"{Name}/{build.Owner.Name}/{build.Repository.Name}/{build.Name}", async () => { - List builds = await BuildClient.GetBuildsAsync( + List builds = await session.BuildClient.GetBuildsAsync( build.Owner.Name, definitions: [int.Parse(build.Id, CultureInfo.InvariantCulture)], top: 10 @@ -306,17 +337,17 @@ internal override async Task UpdateRunAsync(Run run) return; } - EnsureAzureDevOpsClients(); - if (BuildClient == null) + AzureDevOpsSession? session = EnsureAzureDevOpsClients(); + if (session == null) { - Log.Warning($"{Name}: UpdateRunAsync skipped for run '{run.Name}' - BuildClient is null"); + Log.Warning($"{Name}: UpdateRunAsync skipped for run '{run.Name}' - no Azure DevOps session"); return; } Log.Debug($"{Name}: UpdateRunAsync for run '{run.Name}' (ID: {run.Id}) in '{run.Owner.Name}/{run.Repository.Name}/{run.Build.Name}'"); await MakeAzureDevOpsRequestAsync($"{Name}/{run.Owner.Name}/{run.Repository.Name}/{run.Build.Name}/{run.Name}", async () => { - Microsoft.TeamFoundation.Build.WebApi.Build azureBuild = await BuildClient.GetBuildAsync( + Microsoft.TeamFoundation.Build.WebApi.Build azureBuild = await session.BuildClient.GetBuildAsync( run.Owner.Name, int.Parse(run.Id, CultureInfo.InvariantCulture) ).ConfigureAwait(false); diff --git a/BuildMonitor/Providers/CredentialedSessionCache.cs b/BuildMonitor/Providers/CredentialedSessionCache.cs new file mode 100644 index 0000000..fadd15c --- /dev/null +++ b/BuildMonitor/Providers/CredentialedSessionCache.cs @@ -0,0 +1,117 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.BuildMonitor; + +/// +/// Holds the connection-backed session a provider talks to its API through, rebuilding it when the +/// credentials change and handing every caller a snapshot of its own. +/// +/// +/// +/// Provider updates run concurrently: UpdateAsync fans owners, repositories, builds and runs +/// out through Task.WhenAll, and each of those independently asks for the session. Two +/// things go wrong if that is left unsynchronized. +/// +/// +/// Two callers can both decide the session is stale and both build one, so one of the two +/// connections is overwritten without ever being disposed. And a caller that has already read the +/// session, then waits — inside a rate-limit delay, say — can find it replaced with null underneath +/// it by the time it dereferences, which is a rather than the +/// "skipped, no client" path the caller checked for. +/// +/// +/// Both follow from re-reading shared fields, so this hands back the session as a value instead. +/// Callers hold what they were given for the whole of their request, and a replacement built behind +/// them does not disturb a request already in flight. The factory runs under the lock, so building +/// a connection is serialized against another caller building one — which is the point, since that +/// is what the duplicate work and the leak came from. +/// +/// +/// The session type, which owns the connection and disposes it. +internal sealed class CredentialedSessionCache : IDisposable + where TSession : class, IDisposable +{ + private readonly Lock gate = new(); + private readonly Func createSession; + private TSession? session; + private string? lastAccountId; + private string? lastToken; + private bool disposed; + + /// + /// Initializes a new instance of the class. + /// + /// Builds a session from an account id and a token. + internal CredentialedSessionCache(Func createSession) => + this.createSession = createSession; + + /// + /// Gets the number of sessions built so far, which a test uses to show that concurrent callers + /// share one rather than each building their own. + /// + internal int SessionsCreated { get; private set; } + + /// + /// Gets the session for these credentials, building one if the cached session is missing or was + /// built for different credentials. + /// + /// The account the session authenticates against. + /// The token the session authenticates with. + /// The session, to be held by the caller for the whole of its request. + /// The cache has been disposed. + internal TSession Get(string accountId, string token) + { + lock (gate) + { + ObjectDisposedException.ThrowIf(disposed, this); + + if (session is not null && lastAccountId == accountId && lastToken == token) + { + return session; + } + + // Drop the stale session first, so a factory that throws cannot leave credentials + // recorded for a session that was never built. + Invalidate(); + + TSession created = createSession(accountId, token); + session = created; + lastAccountId = accountId; + lastToken = token; + SessionsCreated++; + return created; + } + } + + /// + /// Disposes the cached session and forgets the credentials it was built for, so the next caller + /// builds a fresh one. + /// + internal void Invalidate() + { + lock (gate) + { + session?.Dispose(); + session = null; + lastAccountId = null; + lastToken = null; + } + } + + /// + /// Disposes the cached session. + /// + public void Dispose() + { + lock (gate) + { + if (disposed) + { + return; + } + + Invalidate(); + disposed = true; + } + } +} From a1e6a13be6a0fc7c955ceb1ec6b6606c145e609a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 21 Sep 2026 22:45:39 +0000 Subject: [PATCH 2/4] test: cover the credential check that keeps an unconfigured provider offline EnsureAzureDevOpsClients becomes internal so the branch that decides whether a connection is attempted at all can be tested. A provider with no credentials must report no session rather than reaching the factory, which would otherwise try to authenticate against an organization named by an empty string. Raises new-code coverage from 46.6% to 54.8%. The remaining lines build or use a VssConnection, and VssConnection.GetClient() authenticates against dev.azure.com on the spot, so they cannot run in a test without a live organization and a real token. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SDNXPxBHkugDsgFMcgSdvP --- .gitignore | 18 ++++++ BuildMonitor.Test/AzureDevOpsSessionTests.cs | 58 ++++++++++++++++++++ BuildMonitor/Providers/AzureDevOps.cs | 2 +- 3 files changed, 77 insertions(+), 1 deletion(-) create mode 100644 BuildMonitor.Test/AzureDevOpsSessionTests.cs diff --git a/.gitignore b/.gitignore index dc0470a..e043c9f 100644 --- a/.gitignore +++ b/.gitignore @@ -203,6 +203,11 @@ PublishScripts/ **/[Pp]ackages/* # except build/, which is used as an MSBuild target. !**/[Pp]ackages/build/ +# and except a Unity project's Packages/, which is source: Unity's package manifest and its +# resolved lock file are both meant to be committed, and a NuGet restore folder never contains +# a file by either name. +!**/[Pp]ackages/manifest.json +!**/[Pp]ackages/packages-lock.json # Uncomment if necessary however generally it will be regenerated when needed #!**/[Pp]ackages/repositories.config # NuGet v3's project.json files produces more ignorable files @@ -651,3 +656,16 @@ Temporary Items # ImGui.ini files imgui.ini + +# Game engine projects +# +# Godot: the import cache, and the mono/temp bin+obj a C# build writes. +.godot/ + +# Unity: .meta files are source, not the Visual Studio C++ build artifact that the `*.meta` rule +# further up targets. Unity generates one per asset and it carries the GUID that scenes, prefabs +# and serialized references point at, so ignoring them gives every clone fresh GUIDs and silently +# breaks those references - including for a plug-in whose .dll is itself a build output. This +# negation has to come after that rule to win, and is scoped to the asset tree so the Visual +# Studio artifact stays ignored everywhere else. +!**/[Aa]ssets/**/*.meta diff --git a/BuildMonitor.Test/AzureDevOpsSessionTests.cs b/BuildMonitor.Test/AzureDevOpsSessionTests.cs new file mode 100644 index 0000000..5eec92d --- /dev/null +++ b/BuildMonitor.Test/AzureDevOpsSessionTests.cs @@ -0,0 +1,58 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.BuildMonitor.Test; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +/// +/// Covers the part of the Azure DevOps session lookup that can run without a live organization. +/// +/// +/// Building a session calls VssConnection.GetClient<T>(), which authenticates against +/// dev.azure.com and throws VssUnauthorizedException offline, so everything past the +/// credential check needs a real organization and a real token. The check itself does not, and it +/// is the branch that decides whether a connection is attempted at all. +/// +[TestClass] +public sealed class AzureDevOpsSessionTests +{ + /// + /// A provider with no credentials must report no session rather than attempting a connection. + /// Reaching the factory here would try to authenticate against an organization named by an empty + /// string, so this branch is what keeps an unconfigured provider off the network entirely. + /// + [TestMethod] + public void AProviderWithNoCredentialsHasNoSession() + { + AzureDevOps provider = new(); + + AzureDevOps.AzureDevOpsSession? session = provider.EnsureAzureDevOpsClients(); + + Assert.IsNull(session); + } + + /// + /// Asking twice still attempts nothing, so a repeated update on an unconfigured provider cannot + /// accumulate connections. + /// + [TestMethod] + public void RepeatedLookupsOnAnUnconfiguredProviderStayNull() + { + AzureDevOps provider = new(); + + Assert.IsNull(provider.EnsureAzureDevOpsClients()); + Assert.IsNull(provider.EnsureAzureDevOpsClients()); + } + + /// + /// The provider reports the name the rest of the application keys its configuration off, which is + /// also the JSON discriminator its credentials are persisted under. + /// + [TestMethod] + public void TheProviderReportsItsName() + { + AzureDevOps provider = new(); + + Assert.AreEqual(nameof(AzureDevOps), provider.Name.ToString()); + } +} diff --git a/BuildMonitor/Providers/AzureDevOps.cs b/BuildMonitor/Providers/AzureDevOps.cs index 885ad17..7d9469e 100644 --- a/BuildMonitor/Providers/AzureDevOps.cs +++ b/BuildMonitor/Providers/AzureDevOps.cs @@ -75,7 +75,7 @@ private static AzureDevOpsSession CreateSession(string accountId, string token) /// let a rebuild on another thread null a client between a caller's null check and its use. /// /// The session, or . - private AzureDevOpsSession? EnsureAzureDevOpsClients() + internal AzureDevOpsSession? EnsureAzureDevOpsClients() { string accountId = AccountId.ToString(); string token = Token.ToString(); From 4997dfae4596844f00c48365d4238f7c67af0c2c Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 21 Sep 2026 22:45:48 +0000 Subject: [PATCH 3/4] chore: drop the unrelated .gitignore edit from this branch The local SDK tooling rewrote .gitignore during a build and it was swept into the previous commit. It has nothing to do with this fix, so this restores main's version and keeps the branch's diff to the change. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SDNXPxBHkugDsgFMcgSdvP --- .gitignore | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/.gitignore b/.gitignore index e043c9f..dc0470a 100644 --- a/.gitignore +++ b/.gitignore @@ -203,11 +203,6 @@ PublishScripts/ **/[Pp]ackages/* # except build/, which is used as an MSBuild target. !**/[Pp]ackages/build/ -# and except a Unity project's Packages/, which is source: Unity's package manifest and its -# resolved lock file are both meant to be committed, and a NuGet restore folder never contains -# a file by either name. -!**/[Pp]ackages/manifest.json -!**/[Pp]ackages/packages-lock.json # Uncomment if necessary however generally it will be regenerated when needed #!**/[Pp]ackages/repositories.config # NuGet v3's project.json files produces more ignorable files @@ -656,16 +651,3 @@ Temporary Items # ImGui.ini files imgui.ini - -# Game engine projects -# -# Godot: the import cache, and the mono/temp bin+obj a C# build writes. -.godot/ - -# Unity: .meta files are source, not the Visual Studio C++ build artifact that the `*.meta` rule -# further up targets. Unity generates one per asset and it carries the GUID that scenes, prefabs -# and serialized references point at, so ignoring them gives every clone fresh GUIDs and silently -# breaks those references - including for a plug-in whose .dll is itself a build output. This -# negation has to come after that rule to win, and is scoped to the asset tree so the Visual -# Studio artifact stays ignored everywhere else. -!**/[Aa]ssets/**/*.meta From 9334d9ec7be6044d62eb7ac4bc3e8b1911f77b24 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 21 Sep 2026 22:49:56 +0000 Subject: [PATCH 4/4] test: use Assert.HasCount for the collection-count assertions Takes SonarCloud's four MSTEST0037 findings on this file. Assert.HasCount reports the collection's actual contents when it fails, where Assert.AreEqual on .Count reports only two numbers. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SDNXPxBHkugDsgFMcgSdvP --- BuildMonitor.Test/CredentialedSessionCacheTests.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/BuildMonitor.Test/CredentialedSessionCacheTests.cs b/BuildMonitor.Test/CredentialedSessionCacheTests.cs index 06be3b8..1ff7578 100644 --- a/BuildMonitor.Test/CredentialedSessionCacheTests.cs +++ b/BuildMonitor.Test/CredentialedSessionCacheTests.cs @@ -100,8 +100,8 @@ public void ConcurrentCallersOnAColdCacheShareOneSession() List handedOut = RunConcurrently(_ => cache.Get(AccountId, Token)); Assert.AreEqual(1, cache.SessionsCreated, $"round {round}: more than one session was built"); - Assert.AreEqual(1, created.Count, $"round {round}: more than one session was built"); - Assert.AreEqual(ConcurrentCallers, handedOut.Count); + Assert.HasCount(1, created, $"round {round}: more than one session was built"); + Assert.HasCount(ConcurrentCallers, handedOut); Assert.IsTrue( handedOut.TrueForAll(session => ReferenceEquals(session, handedOut[0])), $"round {round}: callers were handed different sessions"); @@ -132,7 +132,7 @@ public void EverySupersededSessionIsDisposedExactlyOnce() Assert.AreEqual(1, session.Disposals, $"round {round}: a session was disposed {session.Disposals} times"); } - Assert.AreEqual(ConcurrentCallers, handedOut.Count); + Assert.HasCount(ConcurrentCallers, handedOut); } } @@ -160,7 +160,7 @@ public void ASessionHandedToACallerSurvivesARebuildBehindIt() return session.Credentials; }); - Assert.AreEqual(ConcurrentCallers, observed.Count); + Assert.HasCount(ConcurrentCallers, observed); Assert.IsFalse( observed.Exists(string.IsNullOrEmpty), $"round {round}: a caller dereferenced a session it no longer held");