diff --git a/BuildMonitor.Test/AzureDevOpsSessionTests.cs b/BuildMonitor.Test/AzureDevOpsSessionTests.cs index 5eec92d..3191e8b 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(out _); - Assert.IsNull(session); + 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 1ff7578..db11dd7 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, out _); + 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..8fbbb95 100644 --- a/BuildMonitor/Providers/AzureDevOps.cs +++ b/BuildMonitor/Providers/AzureDevOps.cs @@ -67,16 +67,20 @@ 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 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 session, or . - internal AzureDevOpsSession? EnsureAzureDevOpsClients() + /// The leased session, or . + /// The lease, or . + internal CredentialedSessionCache.Lease? EnsureAzureDevOpsClients(out AzureDevOpsSession? session) { + session = null; string accountId = AccountId.ToString(); string token = Token.ToString(); if (string.IsNullOrEmpty(accountId) || string.IsNullOrEmpty(token)) @@ -86,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) { @@ -149,7 +153,7 @@ internal async Task DiscoverProjectsAsync() return; } - AzureDevOpsSession? session = EnsureAzureDevOpsClients(); + using CredentialedSessionCache.Lease? lease = EnsureAzureDevOpsClients(out AzureDevOpsSession? session); if (session == null) { Log.Warning($"{Name}: DiscoverProjectsAsync skipped - no Azure DevOps session after credential update"); @@ -190,7 +194,7 @@ internal override async Task UpdateRepositoriesAsync(Owner owner) return; } - AzureDevOpsSession? session = EnsureAzureDevOpsClients(); + using CredentialedSessionCache.Lease? lease = EnsureAzureDevOpsClients(out AzureDevOpsSession? session); if (session == null) { Log.Warning($"{Name}: UpdateRepositoriesAsync skipped for owner '{owner.Name}' - no Azure DevOps session"); @@ -247,7 +251,7 @@ internal override async Task UpdateBuildsAsync(Repository repository) return; } - AzureDevOpsSession? session = EnsureAzureDevOpsClients(); + 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"); @@ -301,7 +305,7 @@ internal override async Task UpdateBuildAsync(Build build) return; } - AzureDevOpsSession? session = EnsureAzureDevOpsClients(); + 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"); @@ -337,7 +341,7 @@ internal override async Task UpdateRunAsync(Run run) return; } - AzureDevOpsSession? session = EnsureAzureDevOpsClients(); + using CredentialedSessionCache.Lease? lease = EnsureAzureDevOpsClients(out AzureDevOpsSession? session); if (session == null) { Log.Warning($"{Name}: UpdateRunAsync skipped for run '{run.Name}' - no Azure DevOps session"); diff --git a/BuildMonitor/Providers/CredentialedSessionCache.cs b/BuildMonitor/Providers/CredentialedSessionCache.cs index fadd15c..aa6b416 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,79 @@ 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. + /// 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 + /// 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 +145,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); + } + } + } }