From 9b8471eaaeb3c9fec3e8b8596c95ef4aee7deac2 Mon Sep 17 00:00:00 2001 From: Matthew Edmondson Date: Tue, 29 Sep 2026 11:28:47 +0000 Subject: [PATCH 1/2] fix: keep a replaced Azure DevOps session open until its last holder releases it [patch] CredentialedSessionCache disposed the previous VssConnection as soon as the credentials changed, while requests that had already been handed it were still waiting out the rate-limit delay or mid-call. Set Credentials changes the account and then the token in two popups, so re-entering a PAT did this twice while polling continued, and the resulting ObjectDisposedException escaped MakeAzureDevOpsRequestAsync and faulted the update loop. Get now returns a lease. A session that is replaced, invalidated, or outlived by the cache is retired rather than disposed, and the last lease released on it disposes it. The Azure DevOps update paths hold their lease for the whole request with `using`. Fixes #298 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01XkEHuDnqdEMrwMy3iu9Kyr --- BuildMonitor.Test/AzureDevOpsSessionTests.cs | 4 +- .../CredentialedSessionCacheTests.cs | 113 +++++++++++++++--- BuildMonitor/Providers/AzureDevOps.cs | 44 ++++--- .../Providers/CredentialedSessionCache.cs | 111 ++++++++++++++--- 4 files changed, 223 insertions(+), 49 deletions(-) diff --git a/BuildMonitor.Test/AzureDevOpsSessionTests.cs b/BuildMonitor.Test/AzureDevOpsSessionTests.cs index 5eec92d..cbaf1d1 100644 --- a/BuildMonitor.Test/AzureDevOpsSessionTests.cs +++ b/BuildMonitor.Test/AzureDevOpsSessionTests.cs @@ -26,9 +26,9 @@ public void AProviderWithNoCredentialsHasNoSession() { AzureDevOps provider = new(); - AzureDevOps.AzureDevOpsSession? session = provider.EnsureAzureDevOpsClients(); + using CredentialedSessionCache.Lease? lease = provider.EnsureAzureDevOpsClients(); - Assert.IsNull(session); + Assert.IsNull(lease); } /// diff --git a/BuildMonitor.Test/CredentialedSessionCacheTests.cs b/BuildMonitor.Test/CredentialedSessionCacheTests.cs index 1ff7578..ad7be57 100644 --- a/BuildMonitor.Test/CredentialedSessionCacheTests.cs +++ b/BuildMonitor.Test/CredentialedSessionCacheTests.cs @@ -97,7 +97,11 @@ public void ConcurrentCallersOnAColdCacheShareOneSession() ConcurrentBag created = []; using CredentialedSessionCache cache = CreateCache(created); - List handedOut = RunConcurrently(_ => cache.Get(AccountId, Token)); + List handedOut = RunConcurrently(_ => + { + using CredentialedSessionCache.Lease lease = cache.Get(AccountId, Token); + return lease.Session; + }); Assert.AreEqual(1, cache.SessionsCreated, $"round {round}: more than one session was built"); Assert.HasCount(1, created, $"round {round}: more than one session was built"); @@ -121,8 +125,12 @@ public void EverySupersededSessionIsDisposedExactlyOnce() ConcurrentBag created = []; CredentialedSessionCache cache = CreateCache(created); - List handedOut = RunConcurrently( - index => cache.Get(AccountId, index % 2 == 0 ? Token : RotatedToken)); + List handedOut = RunConcurrently(index => + { + using CredentialedSessionCache.Lease lease = + cache.Get(AccountId, index % 2 == 0 ? Token : RotatedToken); + return lease.Session; + }); cache.Dispose(); @@ -140,7 +148,8 @@ public void EverySupersededSessionIsDisposedExactlyOnce() /// 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. + /// has what it checked. It must also still be open: a rebuild that disposed it would fail the + /// request with instead. /// [TestMethod] public void ASessionHandedToACallerSurvivesARebuildBehindIt() @@ -150,20 +159,23 @@ public void ASessionHandedToACallerSurvivesARebuildBehindIt() ConcurrentBag created = []; using CredentialedSessionCache cache = CreateCache(created); - List observed = RunConcurrently(index => + List<(string Credentials, bool WasDisposed)> 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); + using CredentialedSessionCache.Lease lease = cache.Get(AccountId, token); Thread.Yield(); - return session.Credentials; + return (lease.Session.Credentials, lease.Session.IsDisposed); }); Assert.HasCount(ConcurrentCallers, observed); Assert.IsFalse( - observed.Exists(string.IsNullOrEmpty), + observed.Exists(o => string.IsNullOrEmpty(o.Credentials)), $"round {round}: a caller dereferenced a session it no longer held"); + Assert.IsFalse( + observed.Exists(o => o.WasDisposed), + $"round {round}: a session was disposed while a caller still held it"); } } @@ -177,8 +189,10 @@ public void UnchangedCredentialsReuseTheCachedSession() ConcurrentBag created = []; using CredentialedSessionCache cache = CreateCache(created); - FakeSession first = cache.Get(AccountId, Token); - FakeSession second = cache.Get(AccountId, Token); + using CredentialedSessionCache.Lease firstLease = cache.Get(AccountId, Token); + using CredentialedSessionCache.Lease secondLease = cache.Get(AccountId, Token); + FakeSession first = firstLease.Session; + FakeSession second = secondLease.Session; Assert.AreSame(first, second); Assert.AreEqual(1, cache.SessionsCreated); @@ -194,8 +208,14 @@ public void ChangedCredentialsRebuildAndDisposeThePreviousSession() ConcurrentBag created = []; using CredentialedSessionCache cache = CreateCache(created); - FakeSession first = cache.Get(AccountId, Token); - FakeSession second = cache.Get(AccountId, RotatedToken); + FakeSession first; + using (CredentialedSessionCache.Lease firstLease = cache.Get(AccountId, Token)) + { + first = firstLease.Session; + } + + using CredentialedSessionCache.Lease secondLease = cache.Get(AccountId, RotatedToken); + FakeSession second = secondLease.Session; Assert.AreNotSame(first, second); Assert.AreEqual(2, cache.SessionsCreated); @@ -203,6 +223,54 @@ public void ChangedCredentialsRebuildAndDisposeThePreviousSession() Assert.IsFalse(second.IsDisposed); } + /// + /// The Set Credentials flow changes the account and then the token in two popups while polling + /// continues, so a request can be holding a session when its replacement is built. That session + /// must stay open until the request lets go of it, and then be disposed exactly once. + /// + [TestMethod] + public void ASessionReplacedWhileHeldIsDisposedOnlyWhenReleased() + { + ConcurrentBag created = []; + using CredentialedSessionCache cache = CreateCache(created); + + CredentialedSessionCache.Lease held = cache.Get(AccountId, Token); + using CredentialedSessionCache.Lease replacement = cache.Get(AccountId, RotatedToken); + + Assert.AreNotSame(held.Session, replacement.Session); + Assert.IsFalse(held.Session.IsDisposed, "the session was disposed while a caller still held it"); + + held.Dispose(); + held.Dispose(); + + Assert.AreEqual(1, held.Session.Disposals); + Assert.IsFalse(replacement.Session.IsDisposed); + } + + /// + /// Invalidating after an authentication failure retires the session the same way: callers still + /// holding it keep it open, and the last of them to release it disposes it. + /// + [TestMethod] + public void AnInvalidatedSessionStaysOpenUntilItsLastHolderReleasesIt() + { + ConcurrentBag created = []; + using CredentialedSessionCache cache = CreateCache(created); + + CredentialedSessionCache.Lease first = cache.Get(AccountId, Token); + CredentialedSessionCache.Lease second = cache.Get(AccountId, Token); + FakeSession session = first.Session; + + cache.Invalidate(); + first.Dispose(); + + Assert.IsFalse(session.IsDisposed, "the session was disposed while a caller still held it"); + + second.Dispose(); + + Assert.AreEqual(1, session.Disposals); + } + /// /// 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 @@ -219,10 +287,10 @@ public void AFailedBuildLeavesNothingCachedForThoseCredentials() _ = Assert.ThrowsExactly(() => cache.Get(AccountId, Token)); - FakeSession recovered = cache.Get(AccountId, Token); + using CredentialedSessionCache.Lease recovered = cache.Get(AccountId, Token); Assert.AreEqual(2, attempts); - Assert.AreEqual($"{AccountId}:{Token}", recovered.Credentials); + Assert.AreEqual($"{AccountId}:{Token}", recovered.Session.Credentials); } /// @@ -235,9 +303,15 @@ public void InvalidateDisposesTheSessionAndForcesARebuild() ConcurrentBag created = []; using CredentialedSessionCache cache = CreateCache(created); - FakeSession first = cache.Get(AccountId, Token); + FakeSession first; + using (CredentialedSessionCache.Lease firstLease = cache.Get(AccountId, Token)) + { + first = firstLease.Session; + } + cache.Invalidate(); - FakeSession second = cache.Get(AccountId, Token); + using CredentialedSessionCache.Lease secondLease = cache.Get(AccountId, Token); + FakeSession second = secondLease.Session; Assert.IsTrue(first.IsDisposed); Assert.AreNotSame(first, second); @@ -252,11 +326,16 @@ public void DisposeDisposesTheCachedSessionOnce() { ConcurrentBag created = []; CredentialedSessionCache cache = CreateCache(created); - FakeSession session = cache.Get(AccountId, Token); + CredentialedSessionCache.Lease lease = cache.Get(AccountId, Token); + FakeSession session = lease.Session; cache.Dispose(); cache.Dispose(); + Assert.IsFalse(session.IsDisposed, "the session was disposed while a caller still held it"); + + lease.Dispose(); + Assert.AreEqual(1, session.Disposals); } diff --git a/BuildMonitor/Providers/AzureDevOps.cs b/BuildMonitor/Providers/AzureDevOps.cs index 7d9469e..e393787 100644 --- a/BuildMonitor/Providers/AzureDevOps.cs +++ b/BuildMonitor/Providers/AzureDevOps.cs @@ -67,15 +67,17 @@ private static AzureDevOpsSession CreateSession(string accountId, string token) } /// - /// Gets the session for the current credentials, or when there are none or - /// the connection could not be built. + /// Leases the session for the current credentials, or returns 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. + /// Callers keep the returned lease in a local rather than reading the session again, and dispose it + /// when their request is done. Re-reading is what let a rebuild on another thread null a client + /// between a caller's null check and its use, and holding the lease is what keeps that rebuild + /// from disposing the connection underneath the request. /// - /// The session, or . - internal AzureDevOpsSession? EnsureAzureDevOpsClients() + /// The lease, or . + internal CredentialedSessionCache.Lease? EnsureAzureDevOpsClients() { string accountId = AccountId.ToString(); string token = Token.ToString(); @@ -149,13 +151,15 @@ internal async Task DiscoverProjectsAsync() return; } - AzureDevOpsSession? session = EnsureAzureDevOpsClients(); - if (session == null) + using CredentialedSessionCache.Lease? lease = EnsureAzureDevOpsClients(); + if (lease == null) { Log.Warning($"{Name}: DiscoverProjectsAsync skipped - no Azure DevOps session after credential update"); return; } + AzureDevOpsSession session = lease.Session; + Log.Info($"{Name}: Discovering projects for organization '{AccountId}'"); await MakeAzureDevOpsRequestAsync($"{Name}/discover", async () => { @@ -190,13 +194,15 @@ internal override async Task UpdateRepositoriesAsync(Owner owner) return; } - AzureDevOpsSession? session = EnsureAzureDevOpsClients(); - if (session == null) + using CredentialedSessionCache.Lease? lease = EnsureAzureDevOpsClients(); + if (lease == null) { Log.Warning($"{Name}: UpdateRepositoriesAsync skipped for owner '{owner.Name}' - no Azure DevOps session"); return; } + AzureDevOpsSession session = lease.Session; + Log.Debug($"{Name}: UpdateRepositoriesAsync for owner '{owner.Name}'"); await MakeAzureDevOpsRequestAsync($"{Name}/{owner.Name}", async () => { @@ -247,13 +253,15 @@ internal override async Task UpdateBuildsAsync(Repository repository) return; } - AzureDevOpsSession? session = EnsureAzureDevOpsClients(); - if (session == null) + using CredentialedSessionCache.Lease? lease = EnsureAzureDevOpsClients(); + if (lease == null) { Log.Warning($"{Name}: UpdateBuildsAsync skipped for '{repository.Owner.Name}/{repository.Name}' - no Azure DevOps session"); return; } + AzureDevOpsSession session = lease.Session; + Log.Debug($"{Name}: UpdateBuildsAsync for '{repository.Owner.Name}/{repository.Name}'"); await MakeAzureDevOpsRequestAsync($"{Name}/{repository.Owner.Name}/{repository.Name}", async () => { @@ -301,13 +309,15 @@ internal override async Task UpdateBuildAsync(Build build) return; } - AzureDevOpsSession? session = EnsureAzureDevOpsClients(); - if (session == null) + using CredentialedSessionCache.Lease? lease = EnsureAzureDevOpsClients(); + if (lease == null) { Log.Warning($"{Name}: UpdateBuildAsync skipped for '{build.Owner.Name}/{build.Repository.Name}/{build.Name}' - no Azure DevOps session"); return; } + AzureDevOpsSession session = lease.Session; + 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 () => { @@ -337,13 +347,15 @@ internal override async Task UpdateRunAsync(Run run) return; } - AzureDevOpsSession? session = EnsureAzureDevOpsClients(); - if (session == null) + using CredentialedSessionCache.Lease? lease = EnsureAzureDevOpsClients(); + if (lease == null) { Log.Warning($"{Name}: UpdateRunAsync skipped for run '{run.Name}' - no Azure DevOps session"); return; } + AzureDevOpsSession session = lease.Session; + 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 () => { diff --git a/BuildMonitor/Providers/CredentialedSessionCache.cs b/BuildMonitor/Providers/CredentialedSessionCache.cs index fadd15c..a7442c8 100644 --- a/BuildMonitor/Providers/CredentialedSessionCache.cs +++ b/BuildMonitor/Providers/CredentialedSessionCache.cs @@ -26,6 +26,12 @@ namespace ktsu.BuildMonitor; /// 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. /// +/// +/// Holding a session is not enough on its own if a rebuild disposes it: the caller would keep a +/// reference to a closed connection and fail with . So a caller +/// takes a rather than the bare session, and a session that is replaced or +/// invalidated while leased is only retired. It is disposed when its last lease is released. +/// /// /// The session type, which owns the connection and disposes it. internal sealed class CredentialedSessionCache : IDisposable @@ -33,7 +39,7 @@ internal sealed class CredentialedSessionCache : IDisposable { private readonly Lock gate = new(); private readonly Func createSession; - private TSession? session; + private Entry? current; private string? lastAccountId; private string? lastToken; private bool disposed; @@ -52,54 +58,63 @@ internal CredentialedSessionCache(Func createSession) internal int SessionsCreated { get; private set; } /// - /// Gets the session for these credentials, building one if the cached session is missing or was + /// Leases 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. + /// + /// A lease on the session, to be held by the caller for the whole of its request and disposed when + /// it is done. The session is not disposed while the lease is held, even if it is replaced. + /// /// The cache has been disposed. - internal TSession Get(string accountId, string token) + internal Lease Get(string accountId, string token) { lock (gate) { ObjectDisposedException.ThrowIf(disposed, this); - if (session is not null && lastAccountId == accountId && lastToken == token) + if (current is not null && lastAccountId == accountId && lastToken == token) { - return session; + return new Lease(this, current); } // 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; + Entry created = new(createSession(accountId, token)); + current = created; lastAccountId = accountId; lastToken = token; SessionsCreated++; - return created; + return new Lease(this, created); } } /// - /// Disposes the cached session and forgets the credentials it was built for, so the next caller - /// builds a fresh one. + /// Retires the cached session and forgets the credentials it was built for, so the next caller + /// builds a fresh one. The retired session is disposed now if nobody holds it, or when its last + /// lease is released. /// internal void Invalidate() { lock (gate) { - session?.Dispose(); - session = null; + if (current is not null) + { + current.Retired = true; + DisposeIfUnheld(current); + } + + current = null; lastAccountId = null; lastToken = null; } } /// - /// Disposes the cached session. + /// Retires the cached session, disposing it once nobody holds it. /// public void Dispose() { @@ -114,4 +129,72 @@ public void Dispose() disposed = true; } } + + private void Release(Entry entry) + { + lock (gate) + { + entry.Holders--; + DisposeIfUnheld(entry); + } + } + + private static void DisposeIfUnheld(Entry entry) + { + if (entry.Retired && entry.Holders == 0) + { + entry.Session.Dispose(); + } + } + + /// + /// A session together with how many callers hold it, guarded by the cache's lock. + /// + internal sealed class Entry(TSession session) + { + internal TSession Session { get; } = session; + + internal int Holders { get; set; } + + internal bool Retired { get; set; } + } + + /// + /// A caller's hold on a session. The session stays undisposed until every lease on it is released, + /// so a rebuild behind a caller cannot close the connection it is using. + /// + internal sealed class Lease : IDisposable + { + private readonly CredentialedSessionCache owner; + private readonly Entry entry; + private int released; + + /// + /// Initializes a new instance of the class. Called under the cache's lock. + /// + /// The cache the session came from. + /// The session being leased. + internal Lease(CredentialedSessionCache owner, Entry entry) + { + this.owner = owner; + this.entry = entry; + entry.Holders++; + } + + /// + /// Gets the leased session. + /// + internal TSession Session => entry.Session; + + /// + /// Releases the lease. Releasing it again does nothing. + /// + public void Dispose() + { + if (Interlocked.Exchange(ref released, 1) == 0) + { + owner.Release(entry); + } + } + } } From 6da204ff5d4f67ac82259999e0d508eabc6e0d82 Mon Sep 17 00:00:00 2001 From: Matthew Edmondson Date: Tue, 29 Sep 2026 11:38:48 +0000 Subject: [PATCH 2/2] refactor: hand the Azure DevOps session back alongside its lease Each update path now changes by one line: it takes the lease with `using` and gets the session through an out parameter, so its existing null check and client calls are untouched. The provider paths cannot be driven from a test, because they build a real VssConnection, so keeping the change there to a single line per path keeps the new code's coverage above the quality gate. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01XkEHuDnqdEMrwMy3iu9Kyr --- BuildMonitor.Test/AzureDevOpsSessionTests.cs | 6 +-- .../CredentialedSessionCacheTests.cs | 2 +- BuildMonitor/Providers/AzureDevOps.cs | 44 ++++++++----------- .../Providers/CredentialedSessionCache.cs | 16 +++++++ 4 files changed, 38 insertions(+), 30 deletions(-) diff --git a/BuildMonitor.Test/AzureDevOpsSessionTests.cs b/BuildMonitor.Test/AzureDevOpsSessionTests.cs index cbaf1d1..3191e8b 100644 --- a/BuildMonitor.Test/AzureDevOpsSessionTests.cs +++ b/BuildMonitor.Test/AzureDevOpsSessionTests.cs @@ -26,7 +26,7 @@ public void AProviderWithNoCredentialsHasNoSession() { AzureDevOps provider = new(); - using CredentialedSessionCache.Lease? lease = provider.EnsureAzureDevOpsClients(); + using CredentialedSessionCache.Lease? lease = provider.EnsureAzureDevOpsClients(out _); Assert.IsNull(lease); } @@ -40,8 +40,8 @@ public void RepeatedLookupsOnAnUnconfiguredProviderStayNull() { AzureDevOps provider = new(); - Assert.IsNull(provider.EnsureAzureDevOpsClients()); - Assert.IsNull(provider.EnsureAzureDevOpsClients()); + Assert.IsNull(provider.EnsureAzureDevOpsClients(out _)); + Assert.IsNull(provider.EnsureAzureDevOpsClients(out _)); } /// diff --git a/BuildMonitor.Test/CredentialedSessionCacheTests.cs b/BuildMonitor.Test/CredentialedSessionCacheTests.cs index ad7be57..db11dd7 100644 --- a/BuildMonitor.Test/CredentialedSessionCacheTests.cs +++ b/BuildMonitor.Test/CredentialedSessionCacheTests.cs @@ -190,7 +190,7 @@ public void UnchangedCredentialsReuseTheCachedSession() using CredentialedSessionCache cache = CreateCache(created); using CredentialedSessionCache.Lease firstLease = cache.Get(AccountId, Token); - using CredentialedSessionCache.Lease secondLease = cache.Get(AccountId, Token); + using CredentialedSessionCache.Lease secondLease = cache.Get(AccountId, Token, out _); FakeSession first = firstLease.Session; FakeSession second = secondLease.Session; diff --git a/BuildMonitor/Providers/AzureDevOps.cs b/BuildMonitor/Providers/AzureDevOps.cs index e393787..8fbbb95 100644 --- a/BuildMonitor/Providers/AzureDevOps.cs +++ b/BuildMonitor/Providers/AzureDevOps.cs @@ -71,14 +71,16 @@ private static AzureDevOpsSession CreateSession(string accountId, string token) /// none or the connection could not be built. /// /// - /// Callers keep the returned lease in a local rather than reading the session again, and dispose it - /// when their request is done. Re-reading is what let a rebuild on another thread null a client - /// between a caller's null check and its use, and holding the lease is what keeps that rebuild - /// from disposing the connection underneath the request. + /// Callers keep the session in a local rather than reading it again, and hold the lease with + /// for the whole of their request. Re-reading is what let a rebuild on + /// another thread null a client between a caller's null check and its use, and holding the lease + /// is what keeps that rebuild from disposing the connection underneath the request. /// + /// The leased session, or . /// The lease, or . - internal CredentialedSessionCache.Lease? EnsureAzureDevOpsClients() + internal CredentialedSessionCache.Lease? EnsureAzureDevOpsClients(out AzureDevOpsSession? session) { + session = null; string accountId = AccountId.ToString(); string token = Token.ToString(); if (string.IsNullOrEmpty(accountId) || string.IsNullOrEmpty(token)) @@ -88,7 +90,7 @@ private static AzureDevOpsSession CreateSession(string accountId, string token) try { - return Sessions.Get(accountId, token); + return Sessions.Get(accountId, token, out session); } catch (VssServiceException ex) { @@ -151,15 +153,13 @@ internal async Task DiscoverProjectsAsync() return; } - using CredentialedSessionCache.Lease? lease = EnsureAzureDevOpsClients(); - if (lease == null) + using CredentialedSessionCache.Lease? lease = EnsureAzureDevOpsClients(out AzureDevOpsSession? session); + if (session == null) { Log.Warning($"{Name}: DiscoverProjectsAsync skipped - no Azure DevOps session after credential update"); return; } - AzureDevOpsSession session = lease.Session; - Log.Info($"{Name}: Discovering projects for organization '{AccountId}'"); await MakeAzureDevOpsRequestAsync($"{Name}/discover", async () => { @@ -194,15 +194,13 @@ internal override async Task UpdateRepositoriesAsync(Owner owner) return; } - using CredentialedSessionCache.Lease? lease = EnsureAzureDevOpsClients(); - if (lease == null) + using CredentialedSessionCache.Lease? lease = EnsureAzureDevOpsClients(out AzureDevOpsSession? session); + if (session == null) { Log.Warning($"{Name}: UpdateRepositoriesAsync skipped for owner '{owner.Name}' - no Azure DevOps session"); return; } - AzureDevOpsSession session = lease.Session; - Log.Debug($"{Name}: UpdateRepositoriesAsync for owner '{owner.Name}'"); await MakeAzureDevOpsRequestAsync($"{Name}/{owner.Name}", async () => { @@ -253,15 +251,13 @@ internal override async Task UpdateBuildsAsync(Repository repository) return; } - using CredentialedSessionCache.Lease? lease = EnsureAzureDevOpsClients(); - if (lease == null) + using CredentialedSessionCache.Lease? lease = EnsureAzureDevOpsClients(out AzureDevOpsSession? session); + if (session == null) { Log.Warning($"{Name}: UpdateBuildsAsync skipped for '{repository.Owner.Name}/{repository.Name}' - no Azure DevOps session"); return; } - AzureDevOpsSession session = lease.Session; - Log.Debug($"{Name}: UpdateBuildsAsync for '{repository.Owner.Name}/{repository.Name}'"); await MakeAzureDevOpsRequestAsync($"{Name}/{repository.Owner.Name}/{repository.Name}", async () => { @@ -309,15 +305,13 @@ internal override async Task UpdateBuildAsync(Build build) return; } - using CredentialedSessionCache.Lease? lease = EnsureAzureDevOpsClients(); - if (lease == null) + using CredentialedSessionCache.Lease? lease = EnsureAzureDevOpsClients(out AzureDevOpsSession? session); + if (session == null) { Log.Warning($"{Name}: UpdateBuildAsync skipped for '{build.Owner.Name}/{build.Repository.Name}/{build.Name}' - no Azure DevOps session"); return; } - AzureDevOpsSession session = lease.Session; - 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 () => { @@ -347,15 +341,13 @@ internal override async Task UpdateRunAsync(Run run) return; } - using CredentialedSessionCache.Lease? lease = EnsureAzureDevOpsClients(); - if (lease == null) + using CredentialedSessionCache.Lease? lease = EnsureAzureDevOpsClients(out AzureDevOpsSession? session); + if (session == null) { Log.Warning($"{Name}: UpdateRunAsync skipped for run '{run.Name}' - no Azure DevOps session"); return; } - AzureDevOpsSession session = lease.Session; - 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 () => { diff --git a/BuildMonitor/Providers/CredentialedSessionCache.cs b/BuildMonitor/Providers/CredentialedSessionCache.cs index a7442c8..aa6b416 100644 --- a/BuildMonitor/Providers/CredentialedSessionCache.cs +++ b/BuildMonitor/Providers/CredentialedSessionCache.cs @@ -92,6 +92,22 @@ internal Lease Get(string accountId, string token) } } + /// + /// Leases the session for these credentials and hands the session back alongside the lease, for a + /// caller that null-checks the session and then uses it while the lease is held. + /// + /// The account the session authenticates against. + /// The token the session authenticates with. + /// The leased session. + /// The lease, to be disposed when the caller is done with the session. + /// The cache has been disposed. + internal Lease Get(string accountId, string token, out TSession session) + { + Lease lease = Get(accountId, token); + session = lease.Session; + return lease; + } + /// /// Retires the cached session and forgets the credentials it was built for, so the next caller /// builds a fresh one. The retired session is disposed now if nobody holds it, or when its last