Keep a replaced Azure DevOps session open until its last holder releases it [patch]

BuildMonitor.Test/AzureDevOpsSessionTests.cs

@@ -26,9 +26,9 @@ public void AProviderWithNoCredentialsHasNoSession()
 	{
 		AzureDevOps provider = new();
 
-		AzureDevOps.AzureDevOpsSession? session = provider.EnsureAzureDevOpsClients();
+		using CredentialedSessionCache<AzureDevOps.AzureDevOpsSession>.Lease? lease = provider.EnsureAzureDevOpsClients(out _);
 
-		Assert.IsNull(session);
+		Assert.IsNull(lease);
 	}
 
 	/// <summary>
@@ -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 _));
 	}
 
 	/// <summary>

BuildMonitor.Test/CredentialedSessionCacheTests.cs

@@ -97,7 +97,11 @@ public void ConcurrentCallersOnAColdCacheShareOneSession()
 			ConcurrentBag<FakeSession> created = [];
 			using CredentialedSessionCache<FakeSession> cache = CreateCache(created);
 
-			List<FakeSession> handedOut = RunConcurrently(_ => cache.Get(AccountId, Token));
+			List<FakeSession> handedOut = RunConcurrently(_ =>
+			{
+				using CredentialedSessionCache<FakeSession>.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<FakeSession> created = [];
 			CredentialedSessionCache<FakeSession> cache = CreateCache(created);
 
-			List<FakeSession> handedOut = RunConcurrently(
-				index => cache.Get(AccountId, index % 2 == 0 ? Token : RotatedToken));
+			List<FakeSession> handedOut = RunConcurrently(index =>
+			{
+				using CredentialedSessionCache<FakeSession>.Lease lease =
+					cache.Get(AccountId, index % 2 == 0 ? Token : RotatedToken);
+				return lease.Session;
+			});
 
 			cache.Dispose();
 
@@ -140,7 +148,8 @@ public void EverySupersededSessionIsDisposedExactlyOnce()
 	/// The <see cref="NullReferenceException"/> 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 <see cref="ObjectDisposedException"/> instead.
 	/// </summary>
 	[TestMethod]
 	public void ASessionHandedToACallerSurvivesARebuildBehindIt()
@@ -150,20 +159,23 @@ public void ASessionHandedToACallerSurvivesARebuildBehindIt()
 			ConcurrentBag<FakeSession> created = [];
 			using CredentialedSessionCache<FakeSession> cache = CreateCache(created);
 
-			List<string> 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<FakeSession>.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<FakeSession> created = [];
 		using CredentialedSessionCache<FakeSession> cache = CreateCache(created);
 
-		FakeSession first = cache.Get(AccountId, Token);
-		FakeSession second = cache.Get(AccountId, Token);
+		using CredentialedSessionCache<FakeSession>.Lease firstLease = cache.Get(AccountId, Token);
+		using CredentialedSessionCache<FakeSession>.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,15 +208,69 @@ public void ChangedCredentialsRebuildAndDisposeThePreviousSession()
 		ConcurrentBag<FakeSession> created = [];
 		using CredentialedSessionCache<FakeSession> cache = CreateCache(created);
 
-		FakeSession first = cache.Get(AccountId, Token);
-		FakeSession second = cache.Get(AccountId, RotatedToken);
+		FakeSession first;
+		using (CredentialedSessionCache<FakeSession>.Lease firstLease = cache.Get(AccountId, Token))
+		{
+			first = firstLease.Session;
+		}
+
+		using CredentialedSessionCache<FakeSession>.Lease secondLease = cache.Get(AccountId, RotatedToken);
+		FakeSession second = secondLease.Session;
 
 		Assert.AreNotSame(first, second);
 		Assert.AreEqual(2, cache.SessionsCreated);
 		Assert.IsTrue(first.IsDisposed);
 		Assert.IsFalse(second.IsDisposed);
 	}
 
+	/// <summary>
+	/// 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.
+	/// </summary>
+	[TestMethod]
+	public void ASessionReplacedWhileHeldIsDisposedOnlyWhenReleased()
+	{
+		ConcurrentBag<FakeSession> created = [];
+		using CredentialedSessionCache<FakeSession> cache = CreateCache(created);
+
+		CredentialedSessionCache<FakeSession>.Lease held = cache.Get(AccountId, Token);
+		using CredentialedSessionCache<FakeSession>.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);
+	}
+
+	/// <summary>
+	/// 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.
+	/// </summary>
+	[TestMethod]
+	public void AnInvalidatedSessionStaysOpenUntilItsLastHolderReleasesIt()
+	{
+		ConcurrentBag<FakeSession> created = [];
+		using CredentialedSessionCache<FakeSession> cache = CreateCache(created);
+
+		CredentialedSessionCache<FakeSession>.Lease first = cache.Get(AccountId, Token);
+		CredentialedSessionCache<FakeSession>.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);
+	}
+
 	/// <summary>
 	/// 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<InvalidOperationException>(() => cache.Get(AccountId, Token));
 
-		FakeSession recovered = cache.Get(AccountId, Token);
+		using CredentialedSessionCache<FakeSession>.Lease recovered = cache.Get(AccountId, Token);
 
 		Assert.AreEqual(2, attempts);
-		Assert.AreEqual($"{AccountId}:{Token}", recovered.Credentials);
+		Assert.AreEqual($"{AccountId}:{Token}", recovered.Session.Credentials);
 	}
 
 	/// <summary>
@@ -235,9 +303,15 @@ public void InvalidateDisposesTheSessionAndForcesARebuild()
 		ConcurrentBag<FakeSession> created = [];
 		using CredentialedSessionCache<FakeSession> cache = CreateCache(created);
 
-		FakeSession first = cache.Get(AccountId, Token);
+		FakeSession first;
+		using (CredentialedSessionCache<FakeSession>.Lease firstLease = cache.Get(AccountId, Token))
+		{
+			first = firstLease.Session;
+		}
+
 		cache.Invalidate();
-		FakeSession second = cache.Get(AccountId, Token);
+		using CredentialedSessionCache<FakeSession>.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<FakeSession> created = [];
 		CredentialedSessionCache<FakeSession> cache = CreateCache(created);
-		FakeSession session = cache.Get(AccountId, Token);
+		CredentialedSessionCache<FakeSession>.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);
 	}
 

BuildMonitor/Providers/AzureDevOps.cs

@@ -67,16 +67,20 @@
 	}
 
 	/// <summary>
-	/// Gets the session for the current credentials, or <see langword="null"/> when there are none or
-	/// the connection could not be built.
+	/// Leases the session for the current credentials, or returns <see langword="null"/> when there are
+	/// none or the connection could not be built.
 	/// </summary>
 	/// <remarks>
-	/// 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
+	/// <see langword="using"/> 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.
 	/// </remarks>
-	/// <returns>The session, or <see langword="null"/>.</returns>
-	internal AzureDevOpsSession? EnsureAzureDevOpsClients()
+	/// <param name="session">The leased session, or <see langword="null"/>.</param>
+	/// <returns>The lease, or <see langword="null"/>.</returns>
+	internal CredentialedSessionCache<AzureDevOpsSession>.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 @@
 
 		try
 		{
-			return Sessions.Get(accountId, token);
+			return Sessions.Get(accountId, token, out session);
 		}
 		catch (VssServiceException ex)
 		{
@@ -149,7 +153,7 @@
 			return;
 		}
 
-		AzureDevOpsSession? session = EnsureAzureDevOpsClients();
+		using CredentialedSessionCache<AzureDevOpsSession>.Lease? lease = EnsureAzureDevOpsClients(out AzureDevOpsSession? session);
 		if (session == null)
 		{
 			Log.Warning($"{Name}: DiscoverProjectsAsync skipped - no Azure DevOps session after credential update");
@@ -176,7 +180,7 @@
 				Owner owner = Owners[ownerName];
 				RepositoryId repositoryId = project.Id.ToString().As<RepositoryId>();
 				RepositoryName repositoryName = project.Name.As<RepositoryName>();
 				_ = owner.Repositories.GetOrAdd(repositoryId, _ => owner.CreateRepository(repositoryName, repositoryId));
 			}
 			Log.Info($"{Name}: DiscoverProjects found {projectCount} project(s)");
 		}).ConfigureAwait(false);
@@ -190,7 +194,7 @@
 			return;
 		}
 
-		AzureDevOpsSession? session = EnsureAzureDevOpsClients();
+		using CredentialedSessionCache<AzureDevOpsSession>.Lease? lease = EnsureAzureDevOpsClients(out AzureDevOpsSession? session);
 		if (session == null)
 		{
 			Log.Warning($"{Name}: UpdateRepositoriesAsync skipped for owner '{owner.Name}' - no Azure DevOps session");
@@ -218,7 +222,7 @@
 
 					// Get existing repository or create new one
 					bool isNew = !owner.Repositories.ContainsKey(repositoryId);
 					_ = owner.Repositories.GetOrAdd(repositoryId, _ => owner.CreateRepository(repositoryName, repositoryId));
 
 					if (isNew)
 					{
@@ -247,7 +251,7 @@
 			return;
 		}
 
-		AzureDevOpsSession? session = EnsureAzureDevOpsClients();
+		using CredentialedSessionCache<AzureDevOpsSession>.Lease? lease = EnsureAzureDevOpsClients(out AzureDevOpsSession? session);
 		if (session == null)
 		{
 			Log.Warning($"{Name}: UpdateBuildsAsync skipped for '{repository.Owner.Name}/{repository.Name}' - no Azure DevOps session");
@@ -266,7 +270,7 @@
 
 				// Get existing build or create new one
 				bool isNew = false;
 				Build build = repository.Builds.GetOrAdd(buildId, _ =>
 				{
 					isNew = true;
 					return repository.CreateBuild(buildName, buildId);
@@ -301,7 +305,7 @@
 			return;
 		}
 
-		AzureDevOpsSession? session = EnsureAzureDevOpsClients();
+		using CredentialedSessionCache<AzureDevOpsSession>.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 @@
 			return;
 		}
 
-		AzureDevOpsSession? session = EnsureAzureDevOpsClients();
+		using CredentialedSessionCache<AzureDevOpsSession>.Lease? lease = EnsureAzureDevOpsClients(out AzureDevOpsSession? session);
 		if (session == null)
 		{
 			Log.Warning($"{Name}: UpdateRunAsync skipped for run '{run.Name}' - no Azure DevOps session");