From cefbd6c8f7e6fb5e7f71c4df907e94cefff6449c Mon Sep 17 00:00:00 2001 From: Norm Johanson Date: Mon, 3 Nov 2025 17:59:50 -0800 Subject: [PATCH 1/8] Rework the locking in the RefreshingAWSCredentials base class --- .../e11928c9-15fb-4b9e-91d3-91775150d378.json | 12 + .../Credentials/RefreshingAWSCredentials.cs | 218 ++++++++++++------ 2 files changed, 158 insertions(+), 72 deletions(-) create mode 100644 generator/.DevConfigs/e11928c9-15fb-4b9e-91d3-91775150d378.json diff --git a/generator/.DevConfigs/e11928c9-15fb-4b9e-91d3-91775150d378.json b/generator/.DevConfigs/e11928c9-15fb-4b9e-91d3-91775150d378.json new file mode 100644 index 000000000000..f0ca82e6fdb9 --- /dev/null +++ b/generator/.DevConfigs/e11928c9-15fb-4b9e-91d3-91775150d378.json @@ -0,0 +1,12 @@ +{ + "core": { + "updateMinimum": true, + "type": "patch", + "changeLogMessages": [ + "Rework the locking in the RefreshingAWSCredentials" + ], + "backwardIncompatibilitiesToIgnore": [ + "Amazon.Runtime.RefreshingAWSCredentials/MethodAdded" + ] + } +} \ No newline at end of file diff --git a/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs b/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs index afe79d0b5af6..88745e7d5ccb 100644 --- a/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs +++ b/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs @@ -18,6 +18,7 @@ using System; using System.Globalization; using System.Threading; +using System.Threading.Tasks; namespace Amazon.Runtime { @@ -74,19 +75,23 @@ internal bool IsExpiredWithin(TimeSpan preemptExpiryTime) /// Represents the current state of the Credentials. /// /// This can be cleared without synchronization. - protected CredentialsRefreshState currentState; + protected volatile CredentialsRefreshState currentState; #region Private members private TimeSpan _preemptExpiryTime = TimeSpan.FromMinutes(0); + private TimeSpan _expirationBuffer = TimeSpan.FromMinutes(1); + private bool _disposed; - /// - /// Semaphore to control thread access to GetCredentialsAsync method. - /// The semaphore will allow only one thread to generate new credentials and - /// update the current state. - /// private readonly SemaphoreSlim _updateGeneratedCredentialsSemaphore = new SemaphoreSlim(1, 1); + private enum CredentialsLoadState + { + NotLoading, + Loading + } + + private volatile CredentialsLoadState currentLoadState; #endregion @@ -95,8 +100,8 @@ internal bool IsExpiredWithin(TimeSpan preemptExpiryTime) #region Properties /// - /// The time before actual expiration to expire the credentials. - /// Property cannot be set to a negative TimeSpan. + /// If credentials are still valid but the expiration is with the Expiration minus PreemptExpiryTime a + /// background refresh of the credentials will be triggered. /// public TimeSpan PreemptExpiryTime { @@ -109,6 +114,22 @@ public TimeSpan PreemptExpiryTime } } + /// + /// The time substracted from the expiration provided by the credentials provider and then used for determining + /// if the credentials are expired. This provides a buffer to avoid corner case issues of processing time + /// on the client side before the credentials are actually used for signing and validation on the server side. + /// + protected TimeSpan ExpirationBuffer + { + get { return _expirationBuffer; } + set + { + if (value < TimeSpan.Zero) + throw new ArgumentOutOfRangeException("value", "ExpirationBuffer cannot be negative"); + _expirationBuffer = value; + } + } + #endregion #region Override methods @@ -119,51 +140,102 @@ public TimeSpan PreemptExpiryTime /// public override sealed ImmutableCredentials GetCredentials() { - _updateGeneratedCredentialsSemaphore.Wait(); + // We save the currentState as it might be modified or cleared. + var tempState = currentState; - try + if (IsExpired(tempState) || (currentLoadState != CredentialsLoadState.Loading && IsPreemptExpiryWindow(tempState))) { - // We save the currentState as it might be modified or cleared. - var tempState = currentState; - - // If credentials are expired or we don't have any state yet, update - if (ShouldUpdateState(tempState)) + _updateGeneratedCredentialsSemaphore.Wait(); + try { - tempState = GenerateNewCredentials(); - UpdateToGeneratedCredentials(tempState); - currentState = tempState; + // Update the local variable for credentials after acquiring the lock in case another thread got the lock first and updated the current state. + tempState = currentState; + + // If credentials are expired block for credential refresh. + if (IsExpired(tempState)) + { + tempState = GenerateNewCredentials(); + ValidateGeneratedCredentials(tempState); + currentState = tempState; + currentLoadState = CredentialsLoadState.NotLoading; + } + else if (currentLoadState != CredentialsLoadState.Loading && IsPreemptExpiryWindow(tempState)) + { + currentLoadState = CredentialsLoadState.Loading; + _ = BackgroundCredentialsRefreshAsync(); + } + } + finally + { + _updateGeneratedCredentialsSemaphore.Release(); } - - return tempState.Credentials; } - finally + + return tempState.Credentials; + } + + public override sealed async Task GetCredentialsAsync() + { + // We save the currentState as it might be modified or cleared. + var tempState = currentState; + + if (IsExpired(tempState) || (currentLoadState != CredentialsLoadState.Loading && IsPreemptExpiryWindow(tempState))) { - _updateGeneratedCredentialsSemaphore.Release(); + await _updateGeneratedCredentialsSemaphore.WaitAsync().ConfigureAwait(false); + try + { + // Update the local variable for credentials after acquiring the lock in case another thread got the lock first and updated the current state. + tempState = currentState; + + // If credentials are expired block for credential refresh. + if (IsExpired(tempState)) + { + tempState = await GenerateNewCredentialsAsync().ConfigureAwait(false); + ValidateGeneratedCredentials(tempState); + currentState = tempState; + currentLoadState = CredentialsLoadState.NotLoading; + } + else if (currentLoadState != CredentialsLoadState.Loading && IsPreemptExpiryWindow(tempState)) + { + currentLoadState = CredentialsLoadState.Loading; + _ = BackgroundCredentialsRefreshAsync(); + } + } + finally + { + _updateGeneratedCredentialsSemaphore.Release(); + } } + + return tempState.Credentials; } - public override sealed async System.Threading.Tasks.Task GetCredentialsAsync() + private async Task BackgroundCredentialsRefreshAsync() { - await _updateGeneratedCredentialsSemaphore.WaitAsync().ConfigureAwait(false); - try { - // We save the currentState as it might be modified or cleared. - var tempState = currentState; + var newState = await GenerateNewCredentialsAsync().ConfigureAwait(false); + ValidateGeneratedCredentials(newState); - // If credentials are expired, update - if (ShouldUpdateState(tempState)) + // Acquire the lock to atomically update both currentState and currentLoadState + _updateGeneratedCredentialsSemaphore.Wait(); + try { - tempState = await GenerateNewCredentialsAsync().ConfigureAwait(false); - UpdateToGeneratedCredentials(tempState); - currentState = tempState; + currentState = newState; + currentLoadState = CredentialsLoadState.NotLoading; + } + finally + { + _updateGeneratedCredentialsSemaphore.Release(); } - - return tempState.Credentials; } - finally + catch(Exception e) { - _updateGeneratedCredentialsSemaphore.Release(); + _logger.Error(e, "Exception occurred performing background credentials refresh."); + // If any exceptions occur during background refresh, reset the state to NotLoading + // so that future GetCredentials calls can attempt to refresh again. + currentLoadState = CredentialsLoadState.NotLoading; + throw; } } @@ -171,10 +243,10 @@ public override sealed async System.Threading.Tasks.Task G #region Private/protected credential update methods - private void UpdateToGeneratedCredentials(CredentialsRefreshState state) + private void ValidateGeneratedCredentials(CredentialsRefreshState state) { // Check if the new credentials are already expired - if (ShouldUpdateState(state)) + if (IsExpired(state)) { string errorMessage; if (state == null) @@ -191,50 +263,30 @@ private void UpdateToGeneratedCredentials(CredentialsRefreshState state) throw new AmazonClientException(errorMessage); } - // Offset the Expiration by PreemptExpiryTime. This produces the expiration window - // where the credentials should be updated before they actually expire. - state.Expiration -= PreemptExpiryTime; - - if (ShouldUpdateState(state)) - { - // This could happen if the default value of PreemptExpiryTime is - // overridden and set too high such that ShouldUpdate returns true. - _logger.InfoFormat( - "The preempt expiry time is set too high: Current time = {0}, Credentials expiry time = {1}, Preempt expiry time = {2}.", - AWSSDKUtils.CorrectedUtcNow, - state.Expiration, PreemptExpiryTime); - } + state.Expiration -= ExpirationBuffer; } /// - /// Test credentials existence and expiration time - /// should update if: - /// credentials have not been loaded yet - /// it's past the expiration time. At this point currentState.Expiration may - /// have the PreemptExpiryTime baked into to the expiration from a call to - /// UpdateToGeneratedCredentials but it may not if this is new application load. + /// This property has been marked as Obsolete because it is no longer used for determining + /// if credentials should be updated. The boolean ShouldUpdate property did not provide + /// enough information on whether credentials are expired or in the preempt expiry window. /// + [Obsolete("Property is no longer used for determining if credentials should be updated.")] protected bool ShouldUpdate { get { - return ShouldUpdateState(currentState); + return IsExpired(currentState); } } - // Test credentials existence and expiration time - // should update if: - // credentials have not been loaded yet - // it's past the expiration time. At this point currentState.Expiration may - // have the PreemptExpiryTime baked into to the expiration from a call to - // UpdateToGeneratedCredentials but it may not if this is new application - // load. - private bool ShouldUpdateState(CredentialsRefreshState state) + /// + /// Test if the credentials are expired currently expired. + /// + /// + /// + private bool IsExpired(CredentialsRefreshState state) { - // it's past the expiration time. At this point currentState.Expiration may - // have the PreemptExpiryTime baked into to the expiration from a call to - // UpdateToGeneratedCredentials but it may not if this is new application - // load. var isExpired = state?.IsExpiredWithin(TimeSpan.Zero); if (isExpired == true) { @@ -246,6 +298,28 @@ private bool ShouldUpdateState(CredentialsRefreshState state) return isExpired ?? true; } + /// + /// Test if the credentials are in the preempt expiry window. That means the instance currently has credentials and they are not expired but that will expire + /// with in the window of expiration minus PreemptExpiryTime. + /// + /// + /// + private bool IsPreemptExpiryWindow(CredentialsRefreshState state) + { + if (state == null || IsExpired(state)) + return false; + + var isPreemptWindow = state.IsExpiredWithin(PreemptExpiryTime); + if (isPreemptWindow == true) + { + _logger.InfoFormat("Determined refreshing credentials are in window for preempt expiration. Preempt time: {0}, Current time: {1}", + state.Expiration.Add(PreemptExpiryTime).ToUniversalTime().ToString("yyyy-MM-ddTHH:mm:ss.f ffffffK", CultureInfo.InvariantCulture), + AWSSDKUtils.CorrectedUtcNow.ToString("yyyy-MM-ddTHH:mm:ss.fffffffK", CultureInfo.InvariantCulture)); + } + + return isPreemptWindow; + } + /// /// When overridden in a derived class, generates new credentials and new expiration date. /// @@ -263,9 +337,9 @@ protected virtual CredentialsRefreshState GenerateNewCredentials() /// Called on first credentials request and when expiration date is in the past. /// /// - protected virtual System.Threading.Tasks.Task GenerateNewCredentialsAsync() + protected virtual Task GenerateNewCredentialsAsync() { - return System.Threading.Tasks.Task.Run(() => this.GenerateNewCredentials()); + return Task.Run(() => this.GenerateNewCredentials()); } protected virtual void Dispose(bool disposing) From e3d3022dcc0279bb2413499b12a6b03bb0b8e762 Mon Sep 17 00:00:00 2001 From: Norm Johanson Date: Mon, 3 Nov 2025 21:07:19 -0800 Subject: [PATCH 2/8] Add backwardIncompatibilitiesToIgnore to change log --- .../.DevConfigs/e11928c9-15fb-4b9e-91d3-91775150d378.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/generator/.DevConfigs/e11928c9-15fb-4b9e-91d3-91775150d378.json b/generator/.DevConfigs/e11928c9-15fb-4b9e-91d3-91775150d378.json index f0ca82e6fdb9..2ee249102ecc 100644 --- a/generator/.DevConfigs/e11928c9-15fb-4b9e-91d3-91775150d378.json +++ b/generator/.DevConfigs/e11928c9-15fb-4b9e-91d3-91775150d378.json @@ -6,7 +6,8 @@ "Rework the locking in the RefreshingAWSCredentials" ], "backwardIncompatibilitiesToIgnore": [ - "Amazon.Runtime.RefreshingAWSCredentials/MethodAdded" + "Amazon.Runtime.RefreshingAWSCredentials/MethodAdded", + "Amazon.Runtime.RefreshingAWSCredentials/FieldTypeChanged" ] } } \ No newline at end of file From 76e64cb7970282efe017289ce7dbc6282ae27d40 Mon Sep 17 00:00:00 2001 From: Daniel Pinheiro Date: Tue, 4 Nov 2025 20:22:50 -0500 Subject: [PATCH 3/8] refactor: Re-introduce unit tests for background refresh --- .../e11928c9-15fb-4b9e-91d3-91775150d378.json | 8 +- .../InstanceProfileAWSCredentials.cs | 16 +- .../Credentials/RefreshingAWSCredentials.cs | 34 ++- .../Amazon.Util/Internal/ITimeProvider.cs | 42 ++++ sdk/src/Core/GlobalSuppressions.cs | 3 +- .../RefreshingAWSCredentialsTests.cs | 226 ++++++++++++++++++ 6 files changed, 301 insertions(+), 28 deletions(-) create mode 100644 sdk/src/Core/Amazon.Util/Internal/ITimeProvider.cs create mode 100644 sdk/test/NetStandard/UnitTests/Core/Credentials/RefreshingAWSCredentialsTests.cs diff --git a/generator/.DevConfigs/e11928c9-15fb-4b9e-91d3-91775150d378.json b/generator/.DevConfigs/e11928c9-15fb-4b9e-91d3-91775150d378.json index 2ee249102ecc..1f4735227ec2 100644 --- a/generator/.DevConfigs/e11928c9-15fb-4b9e-91d3-91775150d378.json +++ b/generator/.DevConfigs/e11928c9-15fb-4b9e-91d3-91775150d378.json @@ -1,13 +1,13 @@ { "core": { "updateMinimum": true, - "type": "patch", + "type": "minor", "changeLogMessages": [ - "Rework the locking in the RefreshingAWSCredentials" + "Re-introduce background refresh of credentials during their preempt expiry period (https://github.com/aws/aws-sdk-net/issues/4024)" ], - "backwardIncompatibilitiesToIgnore": [ + "backwardIncompatibilitiesToIgnore": [ "Amazon.Runtime.RefreshingAWSCredentials/MethodAdded", - "Amazon.Runtime.RefreshingAWSCredentials/FieldTypeChanged" + "Amazon.Runtime.RefreshingAWSCredentials/FieldTypeChanged" ] } } \ No newline at end of file diff --git a/sdk/src/Core/Amazon.Runtime/Credentials/InstanceProfileAWSCredentials.cs b/sdk/src/Core/Amazon.Runtime/Credentials/InstanceProfileAWSCredentials.cs index fbdb0b85f813..1ff9ea211100 100644 --- a/sdk/src/Core/Amazon.Runtime/Credentials/InstanceProfileAWSCredentials.cs +++ b/sdk/src/Core/Amazon.Runtime/Credentials/InstanceProfileAWSCredentials.cs @@ -91,10 +91,7 @@ protected override CredentialsRefreshState GenerateNewCredentials() // but try again to refresh them in 2 minutes. if (null != _currentRefreshState) { -#pragma warning disable CS0612, CS0618 // Type or member is obsolete - var newExpiryTime = AWSSDKUtils.CorrectedUtcNow + TimeSpan.FromMinutes(2); -#pragma warning restore CS0612,CS0618 // Type or member is obsolete - + var newExpiryTime = _timeProvider.CorrectedUtcNow + TimeSpan.FromMinutes(2); _currentRefreshState = new CredentialsRefreshState(_currentRefreshState.Credentials, newExpiryTime); return _currentRefreshState; } @@ -107,10 +104,7 @@ protected override CredentialsRefreshState GenerateNewCredentials() // use a custom refresh time -#pragma warning disable CS0612, CS0618 // Type or member is obsolete - var newExpiryTime = AWSSDKUtils.CorrectedUtcNow + TimeSpan.FromMinutes(new Random().Next(5, 11)); -#pragma warning restore CS0612, CS0618 // Type or member is obsolete - + var newExpiryTime = _timeProvider.CorrectedUtcNow + TimeSpan.FromMinutes(new Random().Next(5, 11)); _currentRefreshState = new CredentialsRefreshState(newState.Credentials, newExpiryTime); return _currentRefreshState; @@ -175,7 +169,7 @@ protected override async Task GenerateNewCredentialsAsy // but try again to refresh them in 2 minutes. if (null != _currentRefreshState) { - var newExpiryTime = AWSSDKUtils.CorrectedUtcNow + TimeSpan.FromMinutes(2); + var newExpiryTime = _timeProvider.CorrectedUtcNow + TimeSpan.FromMinutes(2); _currentRefreshState = new CredentialsRefreshState(_currentRefreshState.Credentials, newExpiryTime); return _currentRefreshState; } @@ -187,7 +181,7 @@ protected override async Task GenerateNewCredentialsAsy _logger.InfoFormat(_receivedExpiredCredentialsFromIMDS); // use a custom refresh time - var newExpiryTime = AWSSDKUtils.CorrectedUtcNow + TimeSpan.FromMinutes(new Random().Next(5, 11)); + var newExpiryTime = _timeProvider.CorrectedUtcNow + TimeSpan.FromMinutes(new Random().Next(5, 11)); _currentRefreshState = new CredentialsRefreshState(newState.Credentials, newExpiryTime); return _currentRefreshState; @@ -396,7 +390,7 @@ private static Uri InfoUri private CredentialsRefreshState GetEarlyRefreshState(CredentialsRefreshState state) { - DateTime newExpiryTime = AWSSDKUtils.CorrectedUtcNow + _refreshAttemptPeriod + PreemptExpiryTime; + DateTime newExpiryTime = _timeProvider.CorrectedUtcNow + _refreshAttemptPeriod + PreemptExpiryTime; // Use this only if the time is earlier than the default expiration time if (newExpiryTime > state.Expiration) diff --git a/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs b/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs index 88745e7d5ccb..434f2877445c 100644 --- a/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs +++ b/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs @@ -14,7 +14,7 @@ */ using Amazon.Runtime.Internal.Util; -using Amazon.Util; +using Amazon.Util.Internal; using System; using System.Globalization; using System.Threading; @@ -28,6 +28,12 @@ namespace Amazon.Runtime public abstract class RefreshingAWSCredentials : AWSCredentials, IDisposable { private readonly Logger _logger = Logger.GetLogger(typeof(RefreshingAWSCredentials)); + protected readonly ITimeProvider _timeProvider; + + protected RefreshingAWSCredentials() : this(DefaultTimeProvider.Instance) { } + + protected RefreshingAWSCredentials(ITimeProvider timeProvider) + => _timeProvider = timeProvider ?? DefaultTimeProvider.Instance; /// public override DateTime? Expiration @@ -50,22 +56,26 @@ public override DateTime? Expiration /// public class CredentialsRefreshState { + private readonly ITimeProvider _timeProvider; + public ImmutableCredentials Credentials { get; set; } public DateTime Expiration { get; set; } - public CredentialsRefreshState() - { - } + public CredentialsRefreshState() : this(null, default) { } + + public CredentialsRefreshState(ImmutableCredentials credentials, DateTime expiration) + : this (credentials, expiration, DefaultTimeProvider.Instance) { } - public CredentialsRefreshState(ImmutableCredentials credentials, DateTime expiration) + public CredentialsRefreshState(ImmutableCredentials credentials, DateTime expiration, ITimeProvider timeProvider) { Credentials = credentials; Expiration = expiration; + _timeProvider = timeProvider ?? DefaultTimeProvider.Instance; } internal bool IsExpiredWithin(TimeSpan preemptExpiryTime) { - var now = AWSSDKUtils.CorrectedUtcNow; + var now = _timeProvider.CorrectedUtcNow; var exp = Expiration.ToUniversalTime(); return now > exp - preemptExpiryTime; } @@ -119,7 +129,7 @@ public TimeSpan PreemptExpiryTime /// if the credentials are expired. This provides a buffer to avoid corner case issues of processing time /// on the client side before the credentials are actually used for signing and validation on the server side. /// - protected TimeSpan ExpirationBuffer + public TimeSpan ExpirationBuffer { get { return _expirationBuffer; } set @@ -257,7 +267,7 @@ private void ValidateGeneratedCredentials(CredentialsRefreshState state) { errorMessage = string.Format(CultureInfo.InvariantCulture, "The retrieved credentials have already expired: Now = {0}, Credentials expiration = {1}", - AWSSDKUtils.CorrectedUtcNow, state.Expiration); + _timeProvider.CorrectedUtcNow, state.Expiration); } throw new AmazonClientException(errorMessage); @@ -291,8 +301,8 @@ private bool IsExpired(CredentialsRefreshState state) if (isExpired == true) { _logger.InfoFormat("Determined refreshing credentials should update. Expiration time: {0}, Current time: {1}", - state.Expiration.Add(PreemptExpiryTime).ToUniversalTime().ToString("yyyy-MM-ddTHH:mm:ss.f ffffffK", CultureInfo.InvariantCulture), - AWSSDKUtils.CorrectedUtcNow.ToString("yyyy-MM-ddTHH:mm:ss.fffffffK", CultureInfo.InvariantCulture)); + state.Expiration.Subtract(ExpirationBuffer).ToUniversalTime().ToString("yyyy-MM-ddTHH:mm:ss.fffffffK", CultureInfo.InvariantCulture), + _timeProvider.CorrectedUtcNow.ToString("yyyy-MM-ddTHH:mm:ss.fffffffK", CultureInfo.InvariantCulture)); } return isExpired ?? true; @@ -313,8 +323,8 @@ private bool IsPreemptExpiryWindow(CredentialsRefreshState state) if (isPreemptWindow == true) { _logger.InfoFormat("Determined refreshing credentials are in window for preempt expiration. Preempt time: {0}, Current time: {1}", - state.Expiration.Add(PreemptExpiryTime).ToUniversalTime().ToString("yyyy-MM-ddTHH:mm:ss.f ffffffK", CultureInfo.InvariantCulture), - AWSSDKUtils.CorrectedUtcNow.ToString("yyyy-MM-ddTHH:mm:ss.fffffffK", CultureInfo.InvariantCulture)); + state.Expiration.Add(PreemptExpiryTime).ToUniversalTime().ToString("yyyy-MM-ddTHH:mm:ss.fffffffK", CultureInfo.InvariantCulture), + _timeProvider.CorrectedUtcNow.ToString("yyyy-MM-ddTHH:mm:ss.fffffffK", CultureInfo.InvariantCulture)); } return isPreemptWindow; diff --git a/sdk/src/Core/Amazon.Util/Internal/ITimeProvider.cs b/sdk/src/Core/Amazon.Util/Internal/ITimeProvider.cs new file mode 100644 index 000000000000..1bb3a4fe909d --- /dev/null +++ b/sdk/src/Core/Amazon.Util/Internal/ITimeProvider.cs @@ -0,0 +1,42 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +using System; + +namespace Amazon.Util.Internal +{ + /// + /// Interface that can be used to mock the current (corrected) UTC time for testing purposes. + /// + /// The default implementation will use the value from that takes + /// any manual clock correction into account. + /// + public interface ITimeProvider + { + public DateTime CorrectedUtcNow { get; } + } + + /// + /// Default implementation of using . + /// + public sealed class DefaultTimeProvider : ITimeProvider + { + public static readonly DefaultTimeProvider Instance = new(); + + private DefaultTimeProvider() { } + + public DateTime CorrectedUtcNow => AWSSDKUtils.CorrectedUtcNow; + } +} diff --git a/sdk/src/Core/GlobalSuppressions.cs b/sdk/src/Core/GlobalSuppressions.cs index 0eac98387a68..026c03612606 100644 --- a/sdk/src/Core/GlobalSuppressions.cs +++ b/sdk/src/Core/GlobalSuppressions.cs @@ -405,7 +405,8 @@ [module: SuppressMessage("AwsSdkRules", "CR1001:PreventHashAlgorithmCreateRule", Scope = "member", Target = "Amazon.Util.CryptoUtilFactory+CryptoUtil.#CreateSHA256Instance()")] // Visible instance fields -[module: SuppressMessage("Microsoft.Design", "CA1051:DoNotDeclareVisibleInstanceFields", Scope = "member", Target = "Amazon.Runtime.RefreshingAWSCredentials.#currentState")] +[module: SuppressMessage("Microsoft.Design", "CA1051:DoNotDeclareVisibleInstanceFields", Scope = "member", Target = "~F:Amazon.Runtime.RefreshingAWSCredentials.currentState")] +[module: SuppressMessage("Microsoft.Design", "CA1051:DoNotDeclareVisibleInstanceFields", Scope = "member", Target = "~F:Amazon.Runtime.RefreshingAWSCredentials._timeProvider")] // Supression due to IL2CPP error [module: SuppressMessage("Microsoft.Design", "CA1006:DoNotNestGenericTypesInMemberSignatures", Scope = "member", Target = "Amazon.Runtime.Internal.Auth.AWS4Signer.#SortHeaders(System.Collections.Generic.IEnumerable`1>)")] diff --git a/sdk/test/NetStandard/UnitTests/Core/Credentials/RefreshingAWSCredentialsTests.cs b/sdk/test/NetStandard/UnitTests/Core/Credentials/RefreshingAWSCredentialsTests.cs new file mode 100644 index 000000000000..659ca4f7e2e2 --- /dev/null +++ b/sdk/test/NetStandard/UnitTests/Core/Credentials/RefreshingAWSCredentialsTests.cs @@ -0,0 +1,226 @@ +using System; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using Amazon.Runtime; +using Amazon.Util.Internal; +using Xunit; + +namespace UnitTests.NetStandard.Core.Credentials +{ + public sealed class RefreshingAWSCredentialsTests + { + private readonly MockTimeProvider _mockProvider; + private readonly DateTime _baseTimeUtc = new DateTime(1970, 1, 1).ToUniversalTime(); + private readonly TimeSpan _lifetime = TimeSpan.FromMinutes(60); + + public RefreshingAWSCredentialsTests() => _mockProvider = new MockTimeProvider(); + + [Theory] + [InlineData(59.5)] // Credentials are not expired yet but just entered the ExpirationBuffer + [InlineData(60)] // Credentials have just expired + [InlineData(75)] // Credentials are way past expiration + public void ConcurrentCallsToGetCrendentialsOnlyGeneratesNewCredentialsOnce(double instantInMinutes) + { + var mockCredentials = new MockRefreshingAWSCredentials(_lifetime, _mockProvider) + { + PreemptExpiryTime = TimeSpan.FromMinutes(15), + }; + + _mockProvider.CorrectedUtcNow = _baseTimeUtc; + var initialCreds = mockCredentials.GetCredentials(); + + // Prevent GenerateNewCredentials from returning. + _mockProvider.CorrectedUtcNow = _baseTimeUtc + TimeSpan.FromMinutes(instantInMinutes); + mockCredentials.CloseGenerateCredentialsGate(); + var concurrentCredentialTasks = Task.WhenAll( + Enumerable.Range(1, 5).Select(i => Task.Run(() => mockCredentials.GetCredentials())) + ); + + // Allow GenerateNewCredentials to complete. + mockCredentials.OpenGenerateCredentialsGate(); + var allCreds = concurrentCredentialTasks.Result; + Assert.NotEqual(initialCreds, allCreds[0]); + + Assert.Equal(2, mockCredentials.GeneratedTokenCount); + for (var i = 1; i < allCreds.Length; i++) + { + Assert.Equal(allCreds[0], allCreds[i]); + } + } + + [Theory] + [InlineData(59.5)] // Credentials are not expired yet but just entered the ExpirationBuffer + [InlineData(60)] // Credentials have just expired + [InlineData(75)] // Credentials are way past expiration + public async Task ConcurrentCallsToGetCrendentialsOnlyGeneratesNewCredentialsOnceAsync(double instantInMinutes) + { + var mockCredentials = new MockRefreshingAWSCredentials(_lifetime, _mockProvider) + { + PreemptExpiryTime = TimeSpan.FromMinutes(15), + }; + + _mockProvider.CorrectedUtcNow = _baseTimeUtc; + var initialCreds = mockCredentials.GetCredentials(); + + // Prevent GenerateNewCredentials from returning. + _mockProvider.CorrectedUtcNow = _baseTimeUtc + TimeSpan.FromMinutes(instantInMinutes); + mockCredentials.CloseGenerateCredentialsGate(); + var concurrentCredentialTasks = Task.WhenAll( + Enumerable.Range(1, 5).Select(i => mockCredentials.GetCredentialsAsync()) + ); + + // Allow GenerateNewCredentials to complete. + mockCredentials.OpenGenerateCredentialsGate(); + var allCreds = await concurrentCredentialTasks; + + Assert.NotEqual(initialCreds, allCreds[0]); + Assert.Equal(2, mockCredentials.GeneratedTokenCount); + for (var i = 1; i < allCreds.Length; i++) + { + Assert.Equal(allCreds[0], allCreds[i]); + } + } + + [Fact] + public void CredentialsAreRefreshedInImmediatelyWhenExpired() + { + var mockCredentials = new MockRefreshingAWSCredentials(_lifetime, _mockProvider) + { + PreemptExpiryTime = TimeSpan.FromMinutes(15), + }; + + _mockProvider.CorrectedUtcNow = _baseTimeUtc; + var initialCreds = mockCredentials.GetCredentials(); + + _mockProvider.CorrectedUtcNow = _baseTimeUtc + _lifetime; + var credsAfterExpiration = mockCredentials.GetCredentials(); + Assert.NotEqual(initialCreds, credsAfterExpiration); + Assert.Equal(2, mockCredentials.GeneratedTokenCount); + } + + [Fact] + public async Task CredentialsAreRefreshedInImmediatelyWhenExpiredAsync() + { + var mockCredentials = new MockRefreshingAWSCredentials(_lifetime, _mockProvider) + { + PreemptExpiryTime = TimeSpan.FromMinutes(15), + }; + + _mockProvider.CorrectedUtcNow = _baseTimeUtc; + var initialCreds = await mockCredentials.GetCredentialsAsync().ConfigureAwait(false); + + _mockProvider.CorrectedUtcNow = _baseTimeUtc + _lifetime; + var credsAfterExpiration = await mockCredentials.GetCredentialsAsync().ConfigureAwait(false); + Assert.NotEqual(initialCreds, credsAfterExpiration); + Assert.Equal(2, mockCredentials.GeneratedTokenCount); + } + + [Theory] + [InlineData(45.5)] // Credentials just entered the preempt expiry period + [InlineData(50)] + [InlineData(58.5)] // Credentials are still within the preempt expiry period but before the expiration buffer + public void CredentialsAreRefreshedInBackgroundDuringPreemptyExpiryPeriod(double instantInMinutes) + { + var mockCredentials = new MockRefreshingAWSCredentials(_lifetime, _mockProvider) + { + PreemptExpiryTime = TimeSpan.FromMinutes(15), + }; + + _mockProvider.CorrectedUtcNow = _baseTimeUtc; + var initialCreds = mockCredentials.GetCredentials(); + + _mockProvider.CorrectedUtcNow = _baseTimeUtc + TimeSpan.FromMinutes(instantInMinutes); + var previousState = mockCredentials.CurrentState; + var credsDuringPreemptExpiry = mockCredentials.GetCredentials(); + Assert.Equal(initialCreds, credsDuringPreemptExpiry); + + // wait for background refresh to complete + Assert.True(SpinWait.SpinUntil(() => !ReferenceEquals(mockCredentials.CurrentState, previousState), 1_000)); + + var credsAfterRefresh = mockCredentials.GetCredentials(); + Assert.NotEqual(credsAfterRefresh, credsDuringPreemptExpiry); + Assert.Equal(_mockProvider.CorrectedUtcNow + _lifetime - mockCredentials.ExpirationBuffer, mockCredentials.CurrentState.Expiration); + Assert.Equal(2, mockCredentials.GeneratedTokenCount); + } + + [Theory] + [InlineData(45.5)] // Credentials just entered the preempt expiry period + [InlineData(50)] + [InlineData(58.5)] // Credentials are still within the preempt expiry period but before the expiration buffer + public async Task CredentialsAreRefreshedInBackgroundDuringPreemptyExpiryPeriodAsync(double instantInMinutes) + { + var mockCredentials = new MockRefreshingAWSCredentials(_lifetime, _mockProvider) + { + PreemptExpiryTime = TimeSpan.FromMinutes(15), + }; + + _mockProvider.CorrectedUtcNow = _baseTimeUtc; + var initialCreds = await mockCredentials.GetCredentialsAsync().ConfigureAwait(false); + + _mockProvider.CorrectedUtcNow = _baseTimeUtc + TimeSpan.FromMinutes(instantInMinutes); + var previousState = mockCredentials.CurrentState; + var credsDuringPreemptExpiry = await mockCredentials.GetCredentialsAsync().ConfigureAwait(false); + Assert.Equal(initialCreds, credsDuringPreemptExpiry); + + // wait for background refresh to complete + Assert.True(SpinWait.SpinUntil(() => !ReferenceEquals(mockCredentials.CurrentState, previousState), 1_000)); + + var credsAfterRefresh = await mockCredentials.GetCredentialsAsync().ConfigureAwait(false); + Assert.NotEqual(credsAfterRefresh, credsDuringPreemptExpiry); + Assert.Equal(_mockProvider.CorrectedUtcNow + _lifetime - mockCredentials.ExpirationBuffer, mockCredentials.CurrentState.Expiration); + Assert.Equal(2, mockCredentials.GeneratedTokenCount); + } + + private class MockTimeProvider : ITimeProvider + { + public DateTime CorrectedUtcNow { get; set; } + } + + // using a hand-written mock in order to have access to the protected fields + private sealed class MockRefreshingAWSCredentials : RefreshingAWSCredentials + { + private readonly TimeSpan _credentialsLifetime; + private readonly ManualResetEventSlim _generateCredsEvent; + private int _tokenCounter; + + public MockRefreshingAWSCredentials(TimeSpan credentialsLifetime, ITimeProvider timeProvider) + : base(timeProvider) + { + _credentialsLifetime = credentialsLifetime; + _generateCredsEvent = new ManualResetEventSlim(initialState: true); + _tokenCounter = 0; + } + + public CredentialsRefreshState CurrentState => base.currentState; + + public int GeneratedTokenCount => _tokenCounter; + + public bool IsGenerateCredentialsGateClosed => !_generateCredsEvent.IsSet; + + public void OpenGenerateCredentialsGate() + { + _generateCredsEvent.Set(); + } + + public void CloseGenerateCredentialsGate() + { + _generateCredsEvent.Reset(); + } + + protected override CredentialsRefreshState GenerateNewCredentials() + { + _generateCredsEvent.Wait(); + + var credentials = new ImmutableCredentials("access_key_id", "secret_access_key", $"token_{Interlocked.Increment(ref _tokenCounter)}"); + var expiration = _timeProvider.CorrectedUtcNow + _credentialsLifetime; + return new CredentialsRefreshState(credentials, expiration, _timeProvider); + } + + protected override Task GenerateNewCredentialsAsync() + { + return Task.Run(GenerateNewCredentials); + } + } + } +} From ab73e3b8ac7ac65c3a3c5083040b266a4bb3e8c1 Mon Sep 17 00:00:00 2001 From: Daniel Pinheiro Date: Wed, 5 Nov 2025 10:28:02 -0500 Subject: [PATCH 4/8] refactor: Update logging to remove duplicate messages and use correct timestamps --- .../Credentials/RefreshingAWSCredentials.cs | 56 +++++++++++++------ .../RefreshingAWSCredentialsTests.cs | 4 +- 2 files changed, 41 insertions(+), 19 deletions(-) diff --git a/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs b/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs index 434f2877445c..0b08ad8071fd 100644 --- a/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs +++ b/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs @@ -164,6 +164,8 @@ public override sealed ImmutableCredentials GetCredentials() // If credentials are expired block for credential refresh. if (IsExpired(tempState)) { + LogCredentialsExpired(tempState); + tempState = GenerateNewCredentials(); ValidateGeneratedCredentials(tempState); currentState = tempState; @@ -171,6 +173,8 @@ public override sealed ImmutableCredentials GetCredentials() } else if (currentLoadState != CredentialsLoadState.Loading && IsPreemptExpiryWindow(tempState)) { + LogCredentialsPreemptExpiry(tempState); + currentLoadState = CredentialsLoadState.Loading; _ = BackgroundCredentialsRefreshAsync(); } @@ -200,6 +204,8 @@ public override sealed async Task GetCredentialsAsync() // If credentials are expired block for credential refresh. if (IsExpired(tempState)) { + LogCredentialsExpired(tempState); + tempState = await GenerateNewCredentialsAsync().ConfigureAwait(false); ValidateGeneratedCredentials(tempState); currentState = tempState; @@ -207,6 +213,8 @@ public override sealed async Task GetCredentialsAsync() } else if (currentLoadState != CredentialsLoadState.Loading && IsPreemptExpiryWindow(tempState)) { + LogCredentialsPreemptExpiry(tempState); + currentLoadState = CredentialsLoadState.Loading; _ = BackgroundCredentialsRefreshAsync(); } @@ -239,7 +247,7 @@ private async Task BackgroundCredentialsRefreshAsync() _updateGeneratedCredentialsSemaphore.Release(); } } - catch(Exception e) + catch (Exception e) { _logger.Error(e, "Exception occurred performing background credentials refresh."); // If any exceptions occur during background refresh, reset the state to NotLoading @@ -293,41 +301,55 @@ protected bool ShouldUpdate /// /// Test if the credentials are expired currently expired. /// - /// - /// private bool IsExpired(CredentialsRefreshState state) { var isExpired = state?.IsExpiredWithin(TimeSpan.Zero); - if (isExpired == true) + return isExpired ?? true; + } + + private void LogCredentialsExpired(CredentialsRefreshState state) + { + if (state == null) { - _logger.InfoFormat("Determined refreshing credentials should update. Expiration time: {0}, Current time: {1}", - state.Expiration.Subtract(ExpirationBuffer).ToUniversalTime().ToString("yyyy-MM-ddTHH:mm:ss.fffffffK", CultureInfo.InvariantCulture), - _timeProvider.CorrectedUtcNow.ToString("yyyy-MM-ddTHH:mm:ss.fffffffK", CultureInfo.InvariantCulture)); + return; } - return isExpired ?? true; + _logger.InfoFormat( + "Determined refreshing credentials should update. Expiration time: {0}, Current time: {1}", + state.Expiration.ToUniversalTime().ToString("yyyy-MM-ddTHH:mm:ss.fffffffK", CultureInfo.InvariantCulture), + _timeProvider.CorrectedUtcNow.ToString("yyyy-MM-ddTHH:mm:ss.fffffffK", CultureInfo.InvariantCulture) + ); } /// - /// Test if the credentials are in the preempt expiry window. That means the instance currently has credentials and they are not expired but that will expire - /// with in the window of expiration minus PreemptExpiryTime. + /// Test if the credentials are in the preempt expiry window. + /// + /// That means the instance currently has credentials and they are not expired but that will expire + /// within the window of expiration minus PreemptExpiryTime. /// - /// - /// private bool IsPreemptExpiryWindow(CredentialsRefreshState state) { if (state == null || IsExpired(state)) + { return false; + } var isPreemptWindow = state.IsExpiredWithin(PreemptExpiryTime); - if (isPreemptWindow == true) + return isPreemptWindow; + } + + private void LogCredentialsPreemptExpiry(CredentialsRefreshState state) + { + if (state == null) { - _logger.InfoFormat("Determined refreshing credentials are in window for preempt expiration. Preempt time: {0}, Current time: {1}", - state.Expiration.Add(PreemptExpiryTime).ToUniversalTime().ToString("yyyy-MM-ddTHH:mm:ss.fffffffK", CultureInfo.InvariantCulture), - _timeProvider.CorrectedUtcNow.ToString("yyyy-MM-ddTHH:mm:ss.fffffffK", CultureInfo.InvariantCulture)); + return; } - return isPreemptWindow; + _logger.InfoFormat( + "Determined refreshing credentials are in window for preempt expiration. Preempt time: {0}, Current time: {1}", + state.Expiration.Subtract(PreemptExpiryTime).ToUniversalTime().ToString("yyyy-MM-ddTHH:mm:ss.fffffffK", CultureInfo.InvariantCulture), + _timeProvider.CorrectedUtcNow.ToString("yyyy-MM-ddTHH:mm:ss.fffffffK", CultureInfo.InvariantCulture) + ); } /// diff --git a/sdk/test/NetStandard/UnitTests/Core/Credentials/RefreshingAWSCredentialsTests.cs b/sdk/test/NetStandard/UnitTests/Core/Credentials/RefreshingAWSCredentialsTests.cs index 659ca4f7e2e2..6f3376cd2ee6 100644 --- a/sdk/test/NetStandard/UnitTests/Core/Credentials/RefreshingAWSCredentialsTests.cs +++ b/sdk/test/NetStandard/UnitTests/Core/Credentials/RefreshingAWSCredentialsTests.cs @@ -17,7 +17,7 @@ public sealed class RefreshingAWSCredentialsTests public RefreshingAWSCredentialsTests() => _mockProvider = new MockTimeProvider(); [Theory] - [InlineData(59.5)] // Credentials are not expired yet but just entered the ExpirationBuffer + [InlineData(59.5)] // Credentials are not expired yet but just entered the expiration buffer [InlineData(60)] // Credentials have just expired [InlineData(75)] // Credentials are way past expiration public void ConcurrentCallsToGetCrendentialsOnlyGeneratesNewCredentialsOnce(double instantInMinutes) @@ -50,7 +50,7 @@ public void ConcurrentCallsToGetCrendentialsOnlyGeneratesNewCredentialsOnce(doub } [Theory] - [InlineData(59.5)] // Credentials are not expired yet but just entered the ExpirationBuffer + [InlineData(59.5)] // Credentials are not expired yet but just entered the expiration buffer [InlineData(60)] // Credentials have just expired [InlineData(75)] // Credentials are way past expiration public async Task ConcurrentCallsToGetCrendentialsOnlyGeneratesNewCredentialsOnceAsync(double instantInMinutes) From 107d154b2ace82ffa43d142c04f8d39f942b29f6 Mon Sep 17 00:00:00 2001 From: Daniel Pinheiro Date: Wed, 5 Nov 2025 11:46:06 -0500 Subject: [PATCH 5/8] fix: Mark method as static --- .../Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs b/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs index 0b08ad8071fd..57d5ab3d22f8 100644 --- a/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs +++ b/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs @@ -301,7 +301,7 @@ protected bool ShouldUpdate /// /// Test if the credentials are expired currently expired. /// - private bool IsExpired(CredentialsRefreshState state) + private static bool IsExpired(CredentialsRefreshState state) { var isExpired = state?.IsExpiredWithin(TimeSpan.Zero); return isExpired ?? true; From 130b21c7855e88eb665b726f57b14861a3fd98c5 Mon Sep 17 00:00:00 2001 From: Daniel Pinheiro Date: Wed, 5 Nov 2025 13:53:49 -0500 Subject: [PATCH 6/8] refactor: Address Copilot feedback --- .../Credentials/RefreshingAWSCredentials.cs | 8 ++++---- .../Core/Credentials/RefreshingAWSCredentialsTests.cs | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs b/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs index 57d5ab3d22f8..829e13a1fb02 100644 --- a/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs +++ b/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs @@ -110,7 +110,7 @@ private enum CredentialsLoadState #region Properties /// - /// If credentials are still valid but the expiration is with the Expiration minus PreemptExpiryTime a + /// If credentials are still valid but the expiration is within the Expiration minus PreemptExpiryTime a /// background refresh of the credentials will be triggered. /// public TimeSpan PreemptExpiryTime @@ -125,7 +125,7 @@ public TimeSpan PreemptExpiryTime } /// - /// The time substracted from the expiration provided by the credentials provider and then used for determining + /// The time subtracted from the expiration provided by the credentials provider and then used for determining /// if the credentials are expired. This provides a buffer to avoid corner case issues of processing time /// on the client side before the credentials are actually used for signing and validation on the server side. /// @@ -236,7 +236,7 @@ private async Task BackgroundCredentialsRefreshAsync() ValidateGeneratedCredentials(newState); // Acquire the lock to atomically update both currentState and currentLoadState - _updateGeneratedCredentialsSemaphore.Wait(); + await _updateGeneratedCredentialsSemaphore.WaitAsync().ConfigureAwait(false); try { currentState = newState; @@ -299,7 +299,7 @@ protected bool ShouldUpdate } /// - /// Test if the credentials are expired currently expired. + /// Test if the credentials are currently expired. /// private static bool IsExpired(CredentialsRefreshState state) { diff --git a/sdk/test/NetStandard/UnitTests/Core/Credentials/RefreshingAWSCredentialsTests.cs b/sdk/test/NetStandard/UnitTests/Core/Credentials/RefreshingAWSCredentialsTests.cs index 6f3376cd2ee6..6035cfde9284 100644 --- a/sdk/test/NetStandard/UnitTests/Core/Credentials/RefreshingAWSCredentialsTests.cs +++ b/sdk/test/NetStandard/UnitTests/Core/Credentials/RefreshingAWSCredentialsTests.cs @@ -20,7 +20,7 @@ public sealed class RefreshingAWSCredentialsTests [InlineData(59.5)] // Credentials are not expired yet but just entered the expiration buffer [InlineData(60)] // Credentials have just expired [InlineData(75)] // Credentials are way past expiration - public void ConcurrentCallsToGetCrendentialsOnlyGeneratesNewCredentialsOnce(double instantInMinutes) + public void ConcurrentCallsToGetCredentialsOnlyGeneratesNewCredentialsOnce(double instantInMinutes) { var mockCredentials = new MockRefreshingAWSCredentials(_lifetime, _mockProvider) { @@ -53,7 +53,7 @@ public void ConcurrentCallsToGetCrendentialsOnlyGeneratesNewCredentialsOnce(doub [InlineData(59.5)] // Credentials are not expired yet but just entered the expiration buffer [InlineData(60)] // Credentials have just expired [InlineData(75)] // Credentials are way past expiration - public async Task ConcurrentCallsToGetCrendentialsOnlyGeneratesNewCredentialsOnceAsync(double instantInMinutes) + public async Task ConcurrentCallsToGetCredentialsOnlyGeneratesNewCredentialsOnceAsync(double instantInMinutes) { var mockCredentials = new MockRefreshingAWSCredentials(_lifetime, _mockProvider) { @@ -83,7 +83,7 @@ public async Task ConcurrentCallsToGetCrendentialsOnlyGeneratesNewCredentialsOnc } [Fact] - public void CredentialsAreRefreshedInImmediatelyWhenExpired() + public void CredentialsAreRefreshedImmediatelyWhenExpired() { var mockCredentials = new MockRefreshingAWSCredentials(_lifetime, _mockProvider) { @@ -100,7 +100,7 @@ public void CredentialsAreRefreshedInImmediatelyWhenExpired() } [Fact] - public async Task CredentialsAreRefreshedInImmediatelyWhenExpiredAsync() + public async Task CredentialsAreRefreshedImmediatelyWhenExpiredAsync() { var mockCredentials = new MockRefreshingAWSCredentials(_lifetime, _mockProvider) { From 292a4aea044ebcbb28a6a768b8bea6b34921df82 Mon Sep 17 00:00:00 2001 From: Daniel Pinheiro Date: Wed, 5 Nov 2025 16:37:07 -0500 Subject: [PATCH 7/8] refactor: More test cases and documentation updates --- .../Credentials/RefreshingAWSCredentials.cs | 15 +++++- .../RefreshingAWSCredentialsTests.cs | 54 ++++++++++++++++--- 2 files changed, 60 insertions(+), 9 deletions(-) diff --git a/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs b/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs index 829e13a1fb02..abb30c5c6e84 100644 --- a/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs +++ b/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs @@ -95,10 +95,23 @@ internal bool IsExpiredWithin(TimeSpan preemptExpiryTime) private bool _disposed; private readonly SemaphoreSlim _updateGeneratedCredentialsSemaphore = new SemaphoreSlim(1, 1); + + /// + /// Tracks the current state of background credentials refresh. + /// private enum CredentialsLoadState { + /// + /// No background refresh is currently in progress. + /// This is the default state, where credentials are either valid or expired. + /// NotLoading, - Loading + + /// + /// A background refresh is currently in progress. + /// This means we're within the preempt expiry window, and credentials are still valid. + /// + Loading, } private volatile CredentialsLoadState currentLoadState; diff --git a/sdk/test/NetStandard/UnitTests/Core/Credentials/RefreshingAWSCredentialsTests.cs b/sdk/test/NetStandard/UnitTests/Core/Credentials/RefreshingAWSCredentialsTests.cs index 6035cfde9284..e45cd38d4cbe 100644 --- a/sdk/test/NetStandard/UnitTests/Core/Credentials/RefreshingAWSCredentialsTests.cs +++ b/sdk/test/NetStandard/UnitTests/Core/Credentials/RefreshingAWSCredentialsTests.cs @@ -82,35 +82,41 @@ public async Task ConcurrentCallsToGetCredentialsOnlyGeneratesNewCredentialsOnce } } - [Fact] - public void CredentialsAreRefreshedImmediatelyWhenExpired() + [Theory] + [InlineData(15, 1, 60)] // Credentials have just expired. + [InlineData(5, 10, 51)] // Credentials have expired considering the expiration buffer. + public void CredentialsAreRefreshedImmediatelyWhenExpired(int preemptInMinutes, int bufferInMinutes, double instantInMinutes) { var mockCredentials = new MockRefreshingAWSCredentials(_lifetime, _mockProvider) { - PreemptExpiryTime = TimeSpan.FromMinutes(15), + PreemptExpiryTime = TimeSpan.FromMinutes(preemptInMinutes), + ExpirationBuffer = TimeSpan.FromMinutes(bufferInMinutes), }; _mockProvider.CorrectedUtcNow = _baseTimeUtc; var initialCreds = mockCredentials.GetCredentials(); - _mockProvider.CorrectedUtcNow = _baseTimeUtc + _lifetime; + _mockProvider.CorrectedUtcNow = _baseTimeUtc + TimeSpan.FromMinutes(instantInMinutes); var credsAfterExpiration = mockCredentials.GetCredentials(); Assert.NotEqual(initialCreds, credsAfterExpiration); Assert.Equal(2, mockCredentials.GeneratedTokenCount); } - [Fact] - public async Task CredentialsAreRefreshedImmediatelyWhenExpiredAsync() + [Theory] + [InlineData(15, 1, 60)] // Credentials have just expired. + [InlineData(5, 10, 51)] // Credentials have expired considering the expiration buffer. + public async Task CredentialsAreRefreshedImmediatelyWhenExpiredAsync(int preemptInMinutes, int bufferInMinutes, double instantInMinutes) { var mockCredentials = new MockRefreshingAWSCredentials(_lifetime, _mockProvider) { - PreemptExpiryTime = TimeSpan.FromMinutes(15), + PreemptExpiryTime = TimeSpan.FromMinutes(preemptInMinutes), + ExpirationBuffer = TimeSpan.FromMinutes(bufferInMinutes), }; _mockProvider.CorrectedUtcNow = _baseTimeUtc; var initialCreds = await mockCredentials.GetCredentialsAsync().ConfigureAwait(false); - _mockProvider.CorrectedUtcNow = _baseTimeUtc + _lifetime; + _mockProvider.CorrectedUtcNow = _baseTimeUtc + TimeSpan.FromMinutes(instantInMinutes); var credsAfterExpiration = await mockCredentials.GetCredentialsAsync().ConfigureAwait(false); Assert.NotEqual(initialCreds, credsAfterExpiration); Assert.Equal(2, mockCredentials.GeneratedTokenCount); @@ -172,6 +178,38 @@ public async Task CredentialsAreRefreshedInBackgroundDuringPreemptyExpiryPeriodA Assert.Equal(2, mockCredentials.GeneratedTokenCount); } + [Fact] + public void ConcurrentCallsDuringPreemptWindowOnlyGeneratesNewCredentialsOnce() + { + var mockCredentials = new MockRefreshingAWSCredentials(_lifetime, _mockProvider) + { + PreemptExpiryTime = TimeSpan.FromMinutes(15), + }; + + _mockProvider.CorrectedUtcNow = _baseTimeUtc; + var initialCreds = mockCredentials.GetCredentials(); + var previousState = mockCredentials.CurrentState; + + // Move time into preempt expiry period (not expired, but within preempt window). + _mockProvider.CorrectedUtcNow = _baseTimeUtc + TimeSpan.FromMinutes(50); + mockCredentials.CloseGenerateCredentialsGate(); + + // Multiple parallel calls during preempt expiry. + var tasks = Enumerable.Range(1, 5) + .Select(_ => Task.Run(() => mockCredentials.GetCredentials())) + .ToArray(); + mockCredentials.OpenGenerateCredentialsGate(); + Task.WaitAll(tasks); + + // Wait for background refresh to complete. + Assert.True(SpinWait.SpinUntil(() => !ReferenceEquals(mockCredentials.CurrentState, previousState), 1_000)); + + // Only one background refresh should have occurred. + var credsAfterRefresh = mockCredentials.GetCredentials(); + Assert.Equal(2, mockCredentials.GeneratedTokenCount); + Assert.NotEqual(initialCreds, credsAfterRefresh); + } + private class MockTimeProvider : ITimeProvider { public DateTime CorrectedUtcNow { get; set; } From 8e951fcf3f0a2f93e23d97fea5aba64a225f7bba Mon Sep 17 00:00:00 2001 From: Norm Johanson Date: Thu, 6 Nov 2025 11:58:57 -0800 Subject: [PATCH 8/8] Add locking documentation for RefreshingAWSCredentials --- .../Credentials/RefreshingAWSCredentials.cs | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs b/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs index abb30c5c6e84..56e771e9b91f 100644 --- a/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs +++ b/sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs @@ -114,6 +114,8 @@ private enum CredentialsLoadState Loading, } + // Note this is purposefuly marked as volatile since it is modified by multiple threads. Read the comments + // in the GetCredentials method for more information on the locking flow and how this is used. private volatile CredentialsLoadState currentLoadState; #endregion @@ -166,6 +168,31 @@ public override sealed ImmutableCredentials GetCredentials() // We save the currentState as it might be modified or cleared. var tempState = currentState; + // Before acquiring the lock, check if we need to refresh credentials. This is essentially the read only section + // and going into the if block is the costly write section. Majority of threads going through this path will stay + // in the read section avoiding blocking on the semaphore. Execution only goes into the write section when credentials + // are nearing expiration or already expired. + // + // There are two phases of credentials needing to be refreshed: + // + // Credentials are expired. In that case the lock will be acquired and credential refresh will be blocking further + // execution until new credentials are retrieved. Once credentials are retrieved and the lock is released any other + // threads that were blocked due to expired credentials will one by one acquire the lock and see that + // new credentials are present and use those refreshed credentials. + // + // Credential epiration in preempt window. This is the case credentials are still valid but to avoid a later + // blocking expiration refresh a background refresh is triggered. Only one background refresh will be triggered + // which is controlled by setting currentLoadState to Loading while the lock is held. + // Any other threads that come through in either the read or write section in the prempt window will see the + // currentLoadState is Loading and not trigger another background refresh. The current credentials will be returned + // instead of waiting for the background refresh to complete since they are still valid. + // + // The background refresh task is in charge of reseting the currentLoadState to NotLoading once the background refresh is complete + // or an exception happened. In the case of an exception during the background refresh resettting the currentLoadState to NotLoading + // is not done within the scope of the lock. This is safe because the only other place currentLoadState is modified to a different value + // is within the block of code that initiates the background refresh. The execution will never go into that block while a background + // refresh is in progress because the currentLoadState was set to Loading which prevents starting a background refresh. Since + // currentLoadState is potentially modified by multiple threads it has been marked as volatile to ensure the latest value is always read. if (IsExpired(tempState) || (currentLoadState != CredentialsLoadState.Loading && IsPreemptExpiryWindow(tempState))) { _updateGeneratedCredentialsSemaphore.Wait(); @@ -203,6 +230,9 @@ public override sealed ImmutableCredentials GetCredentials() public override sealed async Task GetCredentialsAsync() { + // NOTICE: Before modifying any of the logic read the comments in the synchronous GetCredentials method to + // understand the locking flow. If any changes are required be sure the comments in that method are also updated. + // We save the currentState as it might be modified or cleared. var tempState = currentState; @@ -243,6 +273,9 @@ public override sealed async Task GetCredentialsAsync() private async Task BackgroundCredentialsRefreshAsync() { + // NOTICE: Before modifying any of the logic read the comments in the synchronous GetCredentials method to + // understand the locking flow. If any changes are required be sure the comments in that method are also updated. + try { var newState = await GenerateNewCredentialsAsync().ConfigureAwait(false); @@ -263,8 +296,13 @@ private async Task BackgroundCredentialsRefreshAsync() catch (Exception e) { _logger.Error(e, "Exception occurred performing background credentials refresh."); + // If any exceptions occur during background refresh, reset the state to NotLoading // so that future GetCredentials calls can attempt to refresh again. + // + // This is safe to modify outside of the lock because the only other place currentLoadState is modified + // to a different value is within the block of code that initiates the background refresh. The block + // can never be entered while this background refresh is in progress because currentLoadState was set to Loading. currentLoadState = CredentialsLoadState.NotLoading; throw; }