Use int to guard when metadata update is due

src/Microsoft.IdentityModel.Protocols.OpenIdConnect/Configuration/OpenIdConnectConfigurationValidator.cs

@@ -55,6 +55,7 @@ public ConfigurationValidationResult Validate(OpenIdConnectConfiguration openIdC
                 {
                     ErrorMessage = LogHelper.FormatInvariant(
                         LogMessages.IDX21818,
+                        this,
                         LogHelper.MarkAsNonPII(MinimumNumberOfKeys),
                         LogHelper.MarkAsNonPII(numberOfValidKeys),
                         string.IsNullOrEmpty(convertKeyInfos) ? "None" : convertKeyInfos),

src/Microsoft.IdentityModel.Protocols.OpenIdConnect/LogMessages.cs

@@ -78,7 +78,7 @@ internal static class LogMessages
         internal const string IDX21815 = "IDX21815: Error deserializing json: '{0}' into '{1}'.";
         internal const string IDX21816 = "IDX21816: The number of signing keys must be greater or equal to '{0}'. Value: '{1}'.";
         internal const string IDX21817 = "IDX21817: The OpenIdConnectConfiguration did not contain any JsonWebKeys. This is required to validate the configuration.";
-        internal const string IDX21818 = "IDX21818: The OpenIdConnectConfiguration's valid signing keys cannot be less than {0}. Values: {1}. Invalid keys: {2}";
+        internal const string IDX21818 = "IDX21818: IConfigurationValidator '{0}', requires '{1}' valid signing keys, found: {2}. Invalid keys: {3}";
 #pragma warning restore 1591
     }
 }

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

@@ -18,8 +18,6 @@ namespace Microsoft.IdentityModel.Protocols
     [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1001:TypesThatOwnDisposableFieldsShouldBeDisposable")]
     public class ConfigurationManager<T> : BaseConfigurationManager, IConfigurationManager<T> where T : class
     {
-        private DateTimeOffset _syncAfter = DateTimeOffset.MinValue;
-        private DateTimeOffset _lastRequestRefresh = DateTimeOffset.MinValue;
         private bool _isFirstRefreshRequest = true;
         private readonly SemaphoreSlim _configurationNullLock = new SemaphoreSlim(1);
 
@@ -133,7 +131,8 @@ public ConfigurationManager(string metadataAddress, IConfigurationRetriever<T> c
         /// Obtains an updated version of Configuration.
         /// </summary>
         /// <returns>Configuration of type T.</returns>
-        /// <remarks>If the time since the last call is less than <see cref="BaseConfigurationManager.AutomaticRefreshInterval"/> then <see cref="IConfigurationRetriever{T}.GetConfigurationAsync"/> is not called and the current Configuration is returned.</remarks>
+        /// <remarks>If the time since the last call is less than <see cref="BaseConfigurationManager.AutomaticRefreshInterval"/> then
+        /// <see cref="IConfigurationRetriever{T}.GetConfigurationAsync"/> is not called and the current Configuration is returned.</remarks>
         public async Task<T> GetConfigurationAsync()
         {
             return await GetConfigurationAsync(CancellationToken.None).ConfigureAwait(false);
@@ -144,11 +143,17 @@ public async Task<T> GetConfigurationAsync()
         /// </summary>
         /// <param name="cancel">CancellationToken</param>
         /// <returns>Configuration of type T.</returns>
-        /// <remarks>If the time since the last call is less than <see cref="BaseConfigurationManager.AutomaticRefreshInterval"/> then <see cref="IConfigurationRetriever{T}.GetConfigurationAsync"/> is not called and the current Configuration is returned.</remarks>
+        /// <remarks>If the time since the last call is less than <see cref="BaseConfigurationManager.AutomaticRefreshInterval"/> then
+        /// <see cref="IConfigurationRetriever{T}.GetConfigurationAsync"/> is not called and the current Configuration is returned.</remarks>
         public virtual async Task<T> GetConfigurationAsync(CancellationToken cancel)
         {
-            if (_currentConfiguration != null && _syncAfter > DateTimeOffset.UtcNow)
-                return _currentConfiguration;
+            if (_currentConfiguration != null)
+            {
+                // StartupTime is the time when ConfigurationManager was instantiated.
+                double nextRefresh = _automaticRefreshIntervalInSeconds + _timeInSecondsWhenLastRefreshOccurred;
+                if (nextRefresh > GetSecondsSinceInstanceWasCreated)
+                    return _currentConfiguration;
+            }
 
             Exception fetchMetadataFailure = null;
 
@@ -157,7 +162,7 @@ public virtual async Task<T> GetConfigurationAsync(CancellationToken cancel)
             //   reach out to the metadata endpoint. Since multiple threads could be calling this method
             //   we need to ensure that only one thread is actually fetching the metadata.
             // else
-            //   if task is running, return the current configuration
+            //   if update task is running, return the current configuration
             //   else kick off task to update current configuration
             if (_currentConfiguration == null)
             {
@@ -168,9 +173,11 @@ public virtual async Task<T> GetConfigurationAsync(CancellationToken cancel)
                     return _currentConfiguration;
                 }
 
-#pragma warning disable CA1031 // Do not catch general exception types
                 try
                 {
+                    _configurationRetrieverState = ConfigurationRetrieverRunning;
+                    Interlocked.Increment(ref _numberOfTimesAutomaticRefreshRequested);
+
                     // Don't use the individual CT here, this is a shared operation that shouldn't be affected by an individual's cancellation.
                     // The transport should have it's own timeouts, etc.
                     T configuration = await _configRetriever.GetConfigurationAsync(
@@ -192,7 +199,9 @@ public virtual async Task<T> GetConfigurationAsync(CancellationToken cancel)
 
                     UpdateConfiguration(configuration);
                 }
+#pragma warning disable CA1031 // Do not catch general exception types
                 catch (Exception ex)
+#pragma warning restore CA1031 // Do not catch general exception types
                 {
                     fetchMetadataFailure = ex;
 
@@ -206,15 +215,16 @@ public virtual async Task<T> GetConfigurationAsync(CancellationToken cancel)
                 }
                 finally
                 {
+                    _configurationRetrieverState = ConfigurationRetrieverIdle;
                     _configurationNullLock.Release();
                 }
-#pragma warning restore CA1031 // Do not catch general exception types
             }
             else
             {
                 if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverRunning, ConfigurationRetrieverIdle) == ConfigurationRetrieverIdle)
                 {
-                    _ = Task.Run(UpdateCurrentConfiguration, CancellationToken.None);
+                    Interlocked.Increment(ref _numberOfTimesAutomaticRefreshRequested);
+                    _ = Task.Run(RetrieveAndUpdateConfiguration, CancellationToken.None);
                 }
             }
 
@@ -227,19 +237,18 @@ public virtual async Task<T> GetConfigurationAsync(CancellationToken cancel)
                     LogHelper.FormatInvariant(
                         LogMessages.IDX20803,
                         LogHelper.MarkAsNonPII(MetadataAddress ?? "null"),
-                        LogHelper.MarkAsNonPII(_syncAfter),
+                        LogHelper.MarkAsNonPII(_timeInSecondsWhenLastRefreshOccurred),
                         LogHelper.MarkAsNonPII(fetchMetadataFailure)),
                     fetchMetadataFailure));
         }
 
         /// <summary>
         /// This should be called when the configuration needs to be updated either from RequestRefresh or AutomaticRefresh
         /// The Caller should first check the state checking state using:
-        ///   if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverIdle, ConfigurationRetrieverRunning) != ConfigurationRetrieverRunning).
+        ///   if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverRunning, ConfigurationRetrieverIdle) == ConfigurationRetrieverIdle).
         /// </summary>
-        private void UpdateCurrentConfiguration()
+        private void RetrieveAndUpdateConfiguration()
         {
-#pragma warning disable CA1031 // Do not catch general exception types
             try
             {
                 T configuration = _configRetriever.GetConfigurationAsync(
@@ -265,7 +274,9 @@ private void UpdateCurrentConfiguration()
                         UpdateConfiguration(configuration);
                 }
             }
+#pragma warning disable CA1031 // Do not catch general exception types
             catch (Exception ex)
+#pragma warning restore CA1031 // Do not catch general exception types
             {
                 LogHelper.LogExceptionMessage(
                     new InvalidOperationException(
@@ -279,22 +290,32 @@ private void UpdateCurrentConfiguration()
             {
                 Interlocked.Exchange(ref _configurationRetrieverState, ConfigurationRetrieverIdle);
             }
-#pragma warning restore CA1031 // Do not catch general exception types
         }
 
+        /// <summary>
+        /// Called only when configuration is successfully obtained.
+        /// </summary>
+        /// <param name="configuration">Set <see cref="_currentConfiguration" /> to this value.</param>
         private void UpdateConfiguration(T configuration)
         {
             _currentConfiguration = configuration;
-            _syncAfter = DateTimeUtil.Add(DateTime.UtcNow, AutomaticRefreshInterval +
-                TimeSpan.FromSeconds(new Random().Next((int)AutomaticRefreshInterval.TotalSeconds / 20)));
+            // StartupTime is the time when ConfigurationManager was instantiated.
+            // SecondsSinceInstanceWasCreated is the number of seconds since ConfigurationManager was instantiated.
+            // For automatic refresh, we add a 5% jitter.
+            // Record in seconds when the last time configuration was obtained.
+            double timeInSecondsWhenLastAutomaticRefreshOccurred = GetSecondsSinceInstanceWasCreated +
+                                 ((_automaticRefreshIntervalInSeconds >= int.MaxValue) ? 0 : (_random.Next((int)_maxJitter)));
+
+            // transfer to int in single operation.
+            _timeInSecondsWhenLastRefreshOccurred = (int)((timeInSecondsWhenLastAutomaticRefreshOccurred <= int.MaxValue) ? (int)timeInSecondsWhenLastAutomaticRefreshOccurred : int.MaxValue);
         }
 
         /// <summary>
         /// Obtains an updated version of Configuration.
         /// </summary>
         /// <param name="cancel">CancellationToken</param>
         /// <returns>Configuration of type BaseConfiguration    .</returns>
-        /// <remarks>If the time since the last call is less than <see cref="BaseConfigurationManager.AutomaticRefreshInterval"/> then <see cref="IConfigurationRetriever{T}.GetConfigurationAsync"/> is not called and the current Configuration is returned.</remarks>
+        /// <remarks>If the time since the last call is less than <see cref="BaseConfigurationManager._automaticRefreshIntervalInSeconds"/> then <see cref="IConfigurationRetriever{T}.GetConfigurationAsync"/> is not called and the current Configuration is returned.</remarks>
         public override async Task<BaseConfiguration> GetBaseConfigurationAsync(CancellationToken cancel)
         {
             T obj = await GetConfigurationAsync(cancel).ConfigureAwait(false);
@@ -309,19 +330,30 @@ public override async Task<BaseConfiguration> GetBaseConfigurationAsync(Cancella
         /// </summary>
         public override void RequestRefresh()
         {
-            DateTimeOffset now = DateTimeOffset.UtcNow;
+            if (_configurationRetrieverState == ConfigurationRetrieverRunning)
+                return;
 
-            if (now >= DateTimeUtil.Add(_lastRequestRefresh.UtcDateTime, RefreshInterval) || _isFirstRefreshRequest)
+            double nextRefresh = _requestRefreshIntervalInSeconds + _timeInSecondsWhenLastRequestRefreshWasRequested;
+            if (nextRefresh < GetSecondsSinceInstanceWasCreated || _isFirstRefreshRequest)
             {
                 _isFirstRefreshRequest = false;
                 if (Interlocked.CompareExchange(ref _configurationRetrieverState, ConfigurationRetrieverRunning, ConfigurationRetrieverIdle) == ConfigurationRetrieverIdle)
                 {
-                    _ = Task.Run(UpdateCurrentConfiguration, CancellationToken.None);
-                    _lastRequestRefresh = now;
+                    Interlocked.Increment(ref _numberOfTimesRequestRefreshRequested);
+                    double recordWhenRefreshOccurred = GetSecondsSinceInstanceWasCreated;
+                    // transfer to int in single operation.
+                    _timeInSecondsWhenLastRequestRefreshWasRequested = (int)((recordWhenRefreshOccurred <= int.MaxValue) ? (int)recordWhenRefreshOccurred : int.MaxValue);
+                    _ = Task.Run(RetrieveAndUpdateConfiguration, CancellationToken.None);
                 }
             }
         }
 
+        /// <summary>
+        /// SecondsSinceInstanceWasCreated is the number of seconds since ConfigurationManager was instantiated.
+        /// </summary>
+        /// <returns>double</returns>
+        private double GetSecondsSinceInstanceWasCreated => (DateTimeOffset.UtcNow - StartupTime).TotalSeconds;
+
         /// <summary>
         /// 12 hours is the default time interval that afterwards, <see cref="GetBaseConfigurationAsync(CancellationToken)"/> will obtain new configuration.
         /// </summary>

src/Microsoft.IdentityModel.Protocols/InternalAPI.Unshipped.txt

@@ -0,0 +1 @@
+const Microsoft.IdentityModel.Protocols.LogMessages.IDX20803 = "IDX20803: Unable to obtain configuration from: '{0}'. Will retry in '{1}' seconds. Exception: '{2}'." -> string

src/Microsoft.IdentityModel.Protocols/LogMessages.cs

@@ -21,7 +21,7 @@ internal static class LogMessages
         internal const string IDX20108 = "IDX20108: The address specified '{0}' is not valid as per HTTPS scheme. Please specify an https address for security reasons. If you want to test with http address, set the RequireHttps property  on IDocumentRetriever to false.";
 
         // configuration retrieval errors
-        internal const string IDX20803 = "IDX20803: Unable to obtain configuration from: '{0}'. Will retry at '{1}'. Exception: '{2}'.";
+        internal const string IDX20803 = "IDX20803: Unable to obtain configuration from: '{0}'. Will retry in '{1}' seconds. Exception: '{2}'.";
         internal const string IDX20804 = "IDX20804: Unable to retrieve document from: '{0}'.";
         internal const string IDX20805 = "IDX20805: Obtaining information from metadata endpoint: '{0}'.";
         internal const string IDX20806 = "IDX20806: Unable to obtain an updated configuration from: '{0}'. Returning the current configuration. Exception: '{1}.";

src/Microsoft.IdentityModel.Tokens/BaseConfigurationManager.cs

@@ -16,13 +16,22 @@ namespace Microsoft.IdentityModel.Tokens
     public abstract class BaseConfigurationManager
     {
         private TimeSpan _automaticRefreshInterval = DefaultAutomaticRefreshInterval;
+        internal double _automaticRefreshIntervalInSeconds = DefaultAutomaticRefreshInterval.TotalSeconds;
+        internal double _maxJitter = DefaultAutomaticRefreshInterval.TotalSeconds / 20;
         private TimeSpan _refreshInterval = DefaultRefreshInterval;
+        internal double _requestRefreshIntervalInSeconds = DefaultRefreshInterval.TotalSeconds;
         private TimeSpan _lastKnownGoodLifetime = DefaultLastKnownGoodConfigurationLifetime;
         private BaseConfiguration _lastKnownGoodConfiguration;
         private DateTime? _lastKnownGoodConfigFirstUse;
+        internal Random _random = new();
+
+        // Seconds since the BaseConfigurationManager was created when the last refresh occurred with a %5 random jitter.
+        internal int _timeInSecondsWhenLastRefreshOccurred;
+        internal int _timeInSecondsWhenLastRequestRefreshWasRequested;
 
         internal EventBasedLRUCache<BaseConfiguration, DateTime> _lastKnownGoodConfigurationCache;
 
+
         /// <summary>
         /// Gets or sets the <see cref="TimeSpan"/> that controls how often an automatic metadata refresh should occur.
         /// </summary>
@@ -32,9 +41,59 @@ public TimeSpan AutomaticRefreshInterval
             set
             {
                 if (value < MinimumAutomaticRefreshInterval)
-                    throw LogHelper.LogExceptionMessage(new ArgumentOutOfRangeException(nameof(value), LogHelper.FormatInvariant(LogMessages.IDX10108, LogHelper.MarkAsNonPII(MinimumAutomaticRefreshInterval), LogHelper.MarkAsNonPII(value))));
+                    throw LogHelper.LogExceptionMessage(
+                        new ArgumentOutOfRangeException(
+                            nameof(value),
+                            LogHelper.FormatInvariant(
+                                LogMessages.IDX10108,
+                                LogHelper.MarkAsNonPII(MinimumAutomaticRefreshInterval),
+                                LogHelper.MarkAsNonPII(value))));
 
                 _automaticRefreshInterval = value;
+                Interlocked.Exchange(ref _automaticRefreshIntervalInSeconds, value.TotalSeconds);
+                Interlocked.Exchange(ref _maxJitter, value.TotalSeconds / 20);
+            }
+        }
+
+        /// <summary>
+        /// Records the time this instance was created.
+        /// Used to determine if the automatic refresh or request refresh interval has passed.
+        /// The logic is to remember the number of seconds since startup that the last refresh occurred.
+        /// Store that value in _timeInSecondsWhenLastAutomaticRefreshOccurred or _timeInSecondsWhenLastRequestRefreshOccurred.
+        /// Then compare to (UtcNow - Startup).TotalSeconds.
+        /// The set is used for testing purposes.
+        /// </summary>
+        internal DateTimeOffset StartupTime { get; set; } = DateTimeOffset.UtcNow;
+
+        /// <summary>
+        ///  Incremented each time <see cref="GetBaseConfigurationAsync(CancellationToken)"/> results in a http request.
+        /// </summary>
+        internal long _numberOfTimesAutomaticRefreshRequested;
+
+        /// <summary>
+        /// Thread safe getter for <see cref="_numberOfTimesAutomaticRefreshRequested"/>.
+        /// </summary>
+        internal long NumberOfTimesAutomaticRefreshRequested
+        {
+            get
+            {
+                return Interlocked.Read(ref _numberOfTimesAutomaticRefreshRequested);
+            }
+        }
+
+        /// <summary>
+        /// Incremented each time <see cref="RequestRefresh"/> results in a http request.
+        /// </summary>
+        internal long _numberOfTimesRequestRefreshRequested;
+
+        /// <summary>
+        /// Thread safe getter for <see cref="_numberOfTimesRequestRefreshRequested"/>.
+        /// </summary>
+        internal long NumberOfTimesRequestRefreshRequested
+        {
+            get
+            {
+                return Interlocked.Read(ref _numberOfTimesRequestRefreshRequested);
             }
         }
 
@@ -165,6 +224,7 @@ public TimeSpan RefreshInterval
                     throw LogHelper.LogExceptionMessage(new ArgumentOutOfRangeException(nameof(value), LogHelper.FormatInvariant(LogMessages.IDX10107, LogHelper.MarkAsNonPII(MinimumRefreshInterval), LogHelper.MarkAsNonPII(value))));
 
                 _refreshInterval = value;
+                Interlocked.Exchange(ref _requestRefreshIntervalInSeconds, value.TotalSeconds);
             }
         }