[release/10.0] Fix KeyRingProvider thread pool starvation on cold start (#66737)

* Fix KeyRingProvider thread pool starvation on cold start (#66683)

* fix thread starvation

* address PR comments

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: DeagleGross

src/DataProtection/DataProtection/src/KeyManagement/KeyRingProvider.cs

@@ -360,7 +360,44 @@ private IKeyRing GetCurrentKeyRingCoreNew(DateTime utcNow, bool forceRefresh)
             existingTask = _cacheableKeyRingTask;
             if (existingTask is null)
             {
-                // If there's no existing task, make one now
+                // The forceRefresh path skipped reading _cacheableKeyRing above; read it now.
+                existingCacheableKeyRing ??= Volatile.Read(ref _cacheableKeyRing);
+
+                if (existingCacheableKeyRing is null)
+                {
+                    // Cold start: run the refresh inline on this thread. Scheduling the work
+                    // onto TaskScheduler.Default would risk thread-pool starvation - if all
+                    // available pool threads are blocked waiting on the result, the queued
+                    // work item can't be picked up and the app hangs until the runtime
+                    // injects more threads. See https://github.com/dotnet/aspnetcore/issues/66380.
+                    //
+                    // Other concurrent callers are parked on _cacheableKeyRingLockObj while we
+                    // run; once we publish _cacheableKeyRing and release the lock, they wake up.
+                    // Non-force-refresh callers can then re-check the cache and return the
+                    // freshly populated ring, while forceRefresh callers observe that a cached
+                    // ring now exists and continue through the stale-cache refresh path below.
+                    // This matches the pre-#54675 behavior (https://github.com/dotnet/aspnetcore/pull/54675).
+                    CacheableKeyRing newCacheableKeyRing;
+                    try
+                    {
+                        newCacheableKeyRing = CacheableKeyRingProvider.GetCacheableKeyRing(utcNow);
+                    }
+                    catch (Exception ex)
+                    {
+                        // Cold-start branch: always a "reading" rather than "refreshing" failure, and
+                        // there's no stale ring to extend via WithTemporaryExtendedLifetime.
+                        // The async refresh path handles both concerns. We leave _cacheableKeyRing
+                        // and _cacheableKeyRingTask null, so the next caller retries from scratch.
+                        _logger.ErrorOccurredWhileReadingKeyRing(ex);
+                        throw;
+                    }
+
+                    Volatile.Write(ref _cacheableKeyRing, newCacheableKeyRing);
+                    return newCacheableKeyRing.KeyRing;
+                }
+
+                // Stale ring exists: dispatch the refresh asynchronously and let every other
+                // caller return the stale ring without waiting. This is the perf win from #54675.
                 // PERF: Closing over utcNow substantially slows down the fast case (valid cache) in micro-benchmarks
                 // (closing over `this` for CacheableKeyRingProvider doesn't seem impactful)
                 existingTask = Task.Factory.StartNew(
@@ -390,9 +427,8 @@ private IKeyRing GetCurrentKeyRingCoreNew(DateTime utcNow, bool forceRefresh)
         }
 
         // Prefer a stale cached key ring to blocking
-        if (existingCacheableKeyRing is not null)
+        if (!forceRefresh && existingCacheableKeyRing is not null)
         {
-            Debug.Assert(!forceRefresh, "Consumed cached key ring even though forceRefresh is true");
             Debug.Assert(!CacheableKeyRing.IsValid(existingCacheableKeyRing, utcNow), "Should have returned a valid cached key ring above");
             return existingCacheableKeyRing.KeyRing;
         }

src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/KeyRingProviderTests.cs

@@ -812,6 +812,48 @@ private static ICacheableKeyRingProvider SetupCreateCacheableKeyRingTestAndCreat
         return CreateKeyRingProvider(mockKeyManager.Object, mockDefaultKeyResolver.Object, keyManagementOptions);
     }
 
+    // Regression test for https://github.com/dotnet/aspnetcore/issues/66380.
+    // On cold start (no cached key ring) the new async-refresh code path used to schedule
+    // the refresh work to TaskScheduler.Default and then block every caller on Task.Wait().
+    // When all available thread-pool threads were callers, the queued refresh work could
+    // not be picked up - producing thread-pool starvation and a multi-second freeze on
+    // start until the runtime's hill-climbing injected more threads.
+    //
+    // The starvation can't be reproduced deterministically in a unit test (the runtime's
+    // hill-climbing will eventually rescue the buggy code, so any timing-based assertion
+    // is flaky). We instead verify the underlying invariant that prevents starvation:
+    // on cold start, the calling thread itself must perform the refresh, not a pool worker.
+    // If this invariant holds, starvation is impossible by construction regardless of pool
+    // size or hill-climbing behavior.
+    [Fact]
+    public void GetCurrentKeyRing_ColdStart_RefreshRunsOnCallingThread()
+    {
+        var now = StringToDateTime("2015-03-01 00:00:00Z");
+        var expectedKeyRing = new Mock<IKeyRing>().Object;
+
+        var callingThreadId = Environment.CurrentManagedThreadId;
+        int? refreshThreadId = null;
+
+        var mockCacheableKeyRingProvider = new Mock<ICacheableKeyRingProvider>();
+        mockCacheableKeyRingProvider
+            .Setup(o => o.GetCacheableKeyRing(now))
+            .Returns(() =>
+            {
+                refreshThreadId = Environment.CurrentManagedThreadId;
+                return new CacheableKeyRing(
+                    expirationToken: CancellationToken.None,
+                    expirationTime: now.AddDays(1),
+                    keyRing: expectedKeyRing);
+            });
+
+        var keyRingProvider = CreateKeyRingProvider(mockCacheableKeyRingProvider.Object);
+
+        var keyRing = keyRingProvider.GetCurrentKeyRingCore(now);
+
+        Assert.Same(expectedKeyRing, keyRing);
+        Assert.Equal(callingThreadId, refreshThreadId);
+    }
+
     [Fact]
     public async Task MultipleThreadsSeeExpiredCachedValue()
     {