Adjust for RefreshInterval not influencing AutomaticRefreshInterval. (#2870)

* Do not update _syncAfter unless new configuration was received.
Add random % of AutomaticRefreshInterval to avoid spike in trafic in all cases

* Rename method

* Fixed Interlock ordering

---------

Co-authored-by: id4s <[email protected]>

Author: brentschmaltz

src/Microsoft.IdentityModel.Protocols/Configuration/ConfigurationManager.cs

@@ -27,7 +27,6 @@ public class ConfigurationManager<T> : BaseConfigurationManager, IConfigurationM
         private readonly IConfigurationRetriever<T> _configRetriever;
         private readonly IConfigurationValidator<T> _configValidator;
         private T _currentConfiguration;
-        private TimeSpan _bootstrapRefreshInterval = TimeSpan.FromSeconds(1);
 
         // task states are used to ensure the call to 'update config' (UpdateCurrentConfiguration) is a singleton. Uses Interlocked.CompareExchange.
         // metadata is not being obtained
@@ -169,6 +168,7 @@ public virtual async Task<T> GetConfigurationAsync(CancellationToken cancel)
                     return _currentConfiguration;
                 }
 
+#pragma warning disable CA1031 // Do not catch general exception types
                 try
                 {
                     // Don't use the individual CT here, this is a shared operation that shouldn't be affected by an individual's cancellation.
@@ -190,65 +190,29 @@ public virtual async Task<T> GetConfigurationAsync(CancellationToken cancel)
                                         result.ErrorMessage)));
                     }
 
-                    // Add a random amount between 0 and 5% of AutomaticRefreshInterval jitter to avoid spike traffic to IdentityProvider.
-                    _syncAfter = DateTimeUtil.Add(DateTime.UtcNow, AutomaticRefreshInterval +
-                        TimeSpan.FromSeconds(new Random().Next((int)AutomaticRefreshInterval.TotalSeconds / 20)));
-
-                    _currentConfiguration = configuration;
+                    UpdateConfiguration(configuration);
                 }
                 catch (Exception ex)
                 {
                     fetchMetadataFailure = ex;
 
-                    // In this case configuration was never obtained.
-                    if (_currentConfiguration == null)
-                    {
-                        if (_bootstrapRefreshInterval < RefreshInterval)
-                        {
-                            // Adopt exponential backoff for bootstrap refresh interval with a decorrelated jitter if it is not longer than the refresh interval.
-                            TimeSpan _bootstrapRefreshIntervalWithJitter = TimeSpan.FromSeconds(new Random().Next((int)_bootstrapRefreshInterval.TotalSeconds));
-                            _bootstrapRefreshInterval += _bootstrapRefreshInterval;
-                            _syncAfter = DateTimeUtil.Add(DateTime.UtcNow, _bootstrapRefreshIntervalWithJitter);
-                        }
-                        else
-                        {
-                            _syncAfter = DateTimeUtil.Add(
-                                DateTime.UtcNow,
-                                AutomaticRefreshInterval < RefreshInterval ? AutomaticRefreshInterval : RefreshInterval);
-                        }
-
-                        throw LogHelper.LogExceptionMessage(
-                            new InvalidOperationException(
-                                LogHelper.FormatInvariant(
-                                    LogMessages.IDX20803,
-                                    LogHelper.MarkAsNonPII(MetadataAddress ?? "null"),
-                                    LogHelper.MarkAsNonPII(_syncAfter),
-                                    LogHelper.MarkAsNonPII(ex)),
-                                ex));
-                    }
-                    else
-                    {
-                        _syncAfter = DateTimeUtil.Add(
-                            DateTime.UtcNow,
-                            AutomaticRefreshInterval < RefreshInterval ? AutomaticRefreshInterval : RefreshInterval);
-
-                        LogHelper.LogExceptionMessage(
-                            new InvalidOperationException(
-                                LogHelper.FormatInvariant(
-                                    LogMessages.IDX20806,
-                                    LogHelper.MarkAsNonPII(MetadataAddress ?? "null"),
-                                    LogHelper.MarkAsNonPII(ex)),
-                                ex));
-                    }
+                    LogHelper.LogExceptionMessage(
+                        new InvalidOperationException(
+                             LogHelper.FormatInvariant(
+                                LogMessages.IDX20806,
+                                LogHelper.MarkAsNonPII(MetadataAddress ?? "null"),
+                                LogHelper.MarkAsNonPII(ex)),
+                            ex));
                 }
                 finally
                 {
                     _configurationNullLock.Release();
                 }
+#pragma warning restore CA1031 // Do not catch general exception types
             }
             else
             {
-                if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverIdle, ConfigurationRetrieverRunning) != ConfigurationRetrieverRunning)
+                if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverRunning, ConfigurationRetrieverIdle) == ConfigurationRetrieverIdle)
                 {
                     _ = Task.Run(UpdateCurrentConfiguration, CancellationToken.None);
                 }
@@ -285,7 +249,7 @@ private void UpdateCurrentConfiguration()
 
                 if (_configValidator == null)
                 {
-                    _currentConfiguration = configuration;
+                    UpdateConfiguration(configuration);
                 }
                 else
                 {
@@ -298,7 +262,7 @@ private void UpdateCurrentConfiguration()
                                     LogMessages.IDX20810,
                                     result.ErrorMessage)));
                     else
-                        _currentConfiguration = configuration;
+                        UpdateConfiguration(configuration);
                 }
             }
             catch (Exception ex)
@@ -313,12 +277,18 @@ private void UpdateCurrentConfiguration()
             }
             finally
             {
-                _syncAfter = DateTimeUtil.Add(DateTime.UtcNow, AutomaticRefreshInterval);
                 Interlocked.Exchange(ref _configurationRetrieverState, ConfigurationRetrieverIdle);
             }
 #pragma warning restore CA1031 // Do not catch general exception types
         }
 
+        private void UpdateConfiguration(T configuration)
+        {
+            _currentConfiguration = configuration;
+            _syncAfter = DateTimeUtil.Add(DateTime.UtcNow, AutomaticRefreshInterval +
+                TimeSpan.FromSeconds(new Random().Next((int)AutomaticRefreshInterval.TotalSeconds / 20)));
+        }
+
         /// <summary>
         /// Obtains an updated version of Configuration.
         /// </summary>
@@ -332,8 +302,9 @@ public override async Task<BaseConfiguration> GetBaseConfigurationAsync(Cancella
         }
 
         /// <summary>
-        /// Requests that then next call to <see cref="GetConfigurationAsync()"/> obtain new configuration.
-        /// <para>If it is a first force refresh or the last refresh was greater than <see cref="BaseConfigurationManager.RefreshInterval"/> then the next call to <see cref="GetConfigurationAsync()"/> will retrieve new configuration.</para>
+        /// Triggers updating metadata when:
+        /// <para>1. Called the first time.</para>
+        /// <para>2. The time between when this method was called and DateTimeOffset.Now is greater than <see cref="BaseConfigurationManager.RefreshInterval"/>.</para>
         /// <para>If <see cref="BaseConfigurationManager.RefreshInterval"/> == <see cref="TimeSpan.MaxValue"/> then this method does nothing.</para>
         /// </summary>
         public override void RequestRefresh()
@@ -343,10 +314,10 @@ public override void RequestRefresh()
             if (now >= DateTimeUtil.Add(_lastRequestRefresh.UtcDateTime, RefreshInterval) || _isFirstRefreshRequest)
             {
                 _isFirstRefreshRequest = false;
-                _lastRequestRefresh = now;
-                if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverIdle, ConfigurationRetrieverRunning) != ConfigurationRetrieverRunning)
+                if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverRunning, ConfigurationRetrieverIdle) == ConfigurationRetrieverIdle)
                 {
                     _ = Task.Run(UpdateCurrentConfiguration, CancellationToken.None);
+                    _lastRequestRefresh = now;
                 }
             }
         }

test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/ConfigurationManagerTests.cs

@@ -26,7 +26,7 @@ public class ConfigurationManagerTests
     {
         /// <summary>
         /// This test reaches out the the internet to fetch the OpenIdConnectConfiguration from the specified metadata address.
-        /// There is no validaiton of the configuration. The validation is done in the OpenIdConnectConfigurationSerializationTests.Deserialize
+        /// There is no validation of the configuration. The validation is done in the OpenIdConnectConfigurationSerializationTests.Deserialize
         /// against values obtained 2/2/2024
         /// </summary>
         /// <param name="theoryData"></param>
@@ -209,25 +209,120 @@ public async Task FetchMetadataFailureTest()
             TestUtilities.AssertFailIfErrors(context);
         }
 
+        [Fact]
+        public async Task VerifyInterlockGuardForRequestRefresh()
+        {
+            ManualResetEvent waitEvent = new ManualResetEvent(false);
+            ManualResetEvent signalEvent = new ManualResetEvent(false);
+            InMemoryDocumentRetriever inMemoryDocumentRetriever = InMemoryDocumentRetrieverWithEvents(waitEvent, signalEvent);
+
+            var configurationManager = new ConfigurationManager<OpenIdConnectConfiguration>(
+                    "AADCommonV1Json",
+                    new OpenIdConnectConfigurationRetriever(),
+                    inMemoryDocumentRetriever);
+
+            // populate the configurationManager with AADCommonV1Config
+            TestUtilities.SetField(configurationManager, "_currentConfiguration", OpenIdConfigData.AADCommonV1Config);
+
+            // InMemoryDocumentRetrieverWithEvents will block until waitEvent.Set() is called.
+            // The first RequestRefresh will not have finished before the next RequestRefresh() is called.
+            // The guard '_lastRequestRefresh' will not block as we set it to DateTimeOffset.MinValue.
+            // Interlocked guard will block.
+            // Configuration should be AADCommonV1Config
+            signalEvent.Reset();
+            configurationManager.RequestRefresh();
+
+            // InMemoryDocumentRetrieverWithEvents will signal when it is OK to change the MetadataAddress
+            // otherwise, it may be the case that the MetadataAddress is changed before the previous Task has finished.
+            signalEvent.WaitOne();
+
+            // AADCommonV1Json would have been passed to the the previous retriever, which is blocked on an event.
+            configurationManager.MetadataAddress = "AADCommonV2Json";
+            TestUtilities.SetField(configurationManager, "_lastRequestRefresh", DateTimeOffset.MinValue);
+            configurationManager.RequestRefresh();
+
+            // Set the event to release the lock and let the previous retriever finish.
+            waitEvent.Set();
+
+            // Configuration should be AADCommonV1Config
+            var configuration = await configurationManager.GetConfigurationAsync();
+            Assert.True(configuration.Issuer.Equals(OpenIdConfigData.AADCommonV1Config.Issuer),
+                    $"configuration.Issuer from configurationManager was not as expected," +
+                    $"configuration.Issuer: '{configuration.Issuer}' != expected '{OpenIdConfigData.AADCommonV1Config.Issuer}'.");
+        }
+
+        [Fact]
+        public async Task VerifyInterlockGuardForGetConfigurationAsync()
+        {
+            ManualResetEvent waitEvent = new ManualResetEvent(false);
+            ManualResetEvent signalEvent = new ManualResetEvent(false);
+
+            InMemoryDocumentRetriever inMemoryDocumentRetriever = InMemoryDocumentRetrieverWithEvents(waitEvent, signalEvent);
+            waitEvent.Set();
+
+            var configurationManager = new ConfigurationManager<OpenIdConnectConfiguration>(
+                    "AADCommonV1Json",
+                    new OpenIdConnectConfigurationRetriever(),
+                    inMemoryDocumentRetriever);
+
+            OpenIdConnectConfiguration configuration = await configurationManager.GetConfigurationAsync();
+
+            // InMemoryDocumentRetrieverWithEvents will block until waitEvent.Set() is called.
+            // The GetConfigurationAsync to update config will not have finished before the next GetConfigurationAsync() is called.
+            // The guard '_syncAfter' will not block as we set it to DateTimeOffset.MinValue.
+            // Interlocked guard should block.
+            // Configuration should be AADCommonV1Config
+
+            waitEvent.Reset();
+            signalEvent.Reset();
+
+            TestUtilities.SetField(configurationManager, "_syncAfter", DateTimeOffset.MinValue);
+            await configurationManager.GetConfigurationAsync(CancellationToken.None);
+
+            // InMemoryDocumentRetrieverWithEvents will signal when it is OK to change the MetadataAddress
+            // otherwise, it may be the case that the MetadataAddress is changed before the previous Task has finished.
+            signalEvent.WaitOne();
+
+            // AADCommonV1Json would have been passed to the the previous retriever, which is blocked on an event.
+            configurationManager.MetadataAddress = "AADCommonV2Json";
+            await configurationManager.GetConfigurationAsync(CancellationToken.None);
+
+            // Set the event to release the lock and let the previous retriever finish.
+            waitEvent.Set();
+
+            // Configuration should be AADCommonV1Config
+            configuration = await configurationManager.GetConfigurationAsync();
+            Assert.True(configuration.Issuer.Equals(OpenIdConfigData.AADCommonV1Config.Issuer),
+                    $"configuration.Issuer from configurationManager was not as expected," +
+                    $" configuration.Issuer: '{configuration.Issuer}' != expected: '{OpenIdConfigData.AADCommonV1Config.Issuer}'.");
+        }
+
         [Fact]
         public async Task BootstrapRefreshIntervalTest()
         {
             var context = new CompareContext($"{this}.BootstrapRefreshIntervalTest");
 
-            var documentRetriever = new HttpDocumentRetriever(HttpResponseMessageUtils.SetupHttpClientThatReturns("OpenIdConnectMetadata.json", HttpStatusCode.NotFound));
-            var configManager = new ConfigurationManager<OpenIdConnectConfiguration>("OpenIdConnectMetadata.json", new OpenIdConnectConfigurationRetriever(), documentRetriever) { RefreshInterval = TimeSpan.FromSeconds(2) };
+            var documentRetriever = new HttpDocumentRetriever(
+                HttpResponseMessageUtils.SetupHttpClientThatReturns("OpenIdConnectMetadata.json", HttpStatusCode.NotFound));
+
+            var configManager = new ConfigurationManager<OpenIdConnectConfiguration>(
+                "OpenIdConnectMetadata.json",
+                new OpenIdConnectConfigurationRetriever(),
+                documentRetriever)
+            { RefreshInterval = TimeSpan.FromSeconds(2) };
 
-            // First time to fetch metadata.
+            // ConfigurationManager._syncAfter is set to DateTimeOffset.MinValue on startup
+            // If obtaining the metadata fails due to error, the value should not change
             try
             {
                 var configuration = await configManager.GetConfigurationAsync();
             }
             catch (Exception firstFetchMetadataFailure)
             {
-                // Refresh interval is BootstrapRefreshInterval
-                var syncAfter = configManager.GetType().GetField("_syncAfter", BindingFlags.NonPublic | BindingFlags.Instance).GetValue(configManager);
-                if ((DateTimeOffset)syncAfter > DateTime.UtcNow + TimeSpan.FromSeconds(2))
-                    context.AddDiff($"Expected the refresh interval is longer than 2 seconds.");
+                // _syncAfter should not have been changed, because the fetch failed.
+                var syncAfter = TestUtilities.GetField(configManager, "_syncAfter");
+                if ((DateTimeOffset)syncAfter != DateTimeOffset.MinValue)
+                    context.AddDiff($"ConfigurationManager._syncAfter: '{syncAfter}' should equal '{DateTimeOffset.MinValue}'.");
 
                 if (firstFetchMetadataFailure.InnerException == null)
                     context.AddDiff($"Expected exception to contain inner exception for fetch metadata failure.");
@@ -243,11 +338,10 @@ public async Task BootstrapRefreshIntervalTest()
                     if (secondFetchMetadataFailure.InnerException == null)
                         context.AddDiff($"Expected exception to contain inner exception for fetch metadata failure.");
 
-                    syncAfter = configManager.GetType().GetField("_syncAfter", BindingFlags.NonPublic | BindingFlags.Instance).GetValue(configManager);
-
-                    // Refresh interval is RefreshInterval
-                    if ((DateTimeOffset)syncAfter > DateTime.UtcNow + configManager.RefreshInterval)
-                        context.AddDiff($"Expected the refresh interval is longer than 2 seconds.");
+                    // _syncAfter should not have been changed, because the fetch failed.
+                    syncAfter = TestUtilities.GetField(configManager, "_syncAfter");
+                    if ((DateTimeOffset)syncAfter != DateTimeOffset.MinValue)
+                        context.AddDiff($"ConfigurationManager._syncAfter: '{syncAfter}' should equal '{DateTimeOffset.MinValue}'.");
 
                     IdentityComparer.AreEqual(firstFetchMetadataFailure, secondFetchMetadataFailure, context);
                 }
@@ -808,6 +902,20 @@ public static TheoryData<ConfigurationManagerTheoryData<OpenIdConnectConfigurati
                 { "https://login.microsoftonline.com/common/discovery/v2.0/keys", OpenIdConfigData.AADCommonV2JwksString }
             });
 
+        private static InMemoryDocumentRetriever InMemoryDocumentRetrieverWithEvents(ManualResetEvent waitEvent, ManualResetEvent signalEvent)
+        {
+            return new InMemoryDocumentRetriever(
+                new Dictionary<string, string>
+                {
+                    { "AADCommonV1Json", OpenIdConfigData.AADCommonV1Json },
+                    { "https://login.microsoftonline.com/common/discovery/keys", OpenIdConfigData.AADCommonV1JwksString },
+                    { "AADCommonV2Json", OpenIdConfigData.AADCommonV2Json },
+                    { "https://login.microsoftonline.com/common/discovery/v2.0/keys", OpenIdConfigData.AADCommonV2JwksString }
+                },
+                waitEvent,
+                signalEvent);
+        }
+
         public class ConfigurationManagerTheoryData<T> : TheoryDataBase where T : class
         {
             public ConfigurationManager<T> ConfigurationManager { get; set; }

test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/OpenIdConnectProtocolValidatorTests.cs

@@ -12,11 +12,8 @@
 using System.Threading.Tasks;
 using Microsoft.IdentityModel.TestUtils;
 using Microsoft.IdentityModel.Tokens;
-using Newtonsoft.Json;
 using Xunit;
 
-#pragma warning disable CS3016 // Arrays as attribute arguments is not CLS-compliant
-
 namespace Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests
 {
     /// <summary>
@@ -1813,5 +1810,3 @@ protected override void OnEventWritten(EventWrittenEventArgs eventData)
         }
     }
 }
-
-#pragma warning restore CS3016 // Arrays as attribute arguments is not CLS-compliant