Skip to content

Add resilient credential refresh behaviour - #3399

Open
smilkuri wants to merge 3 commits into
masterfrom
feat/hosm
Open

smilkuri wants to merge 3 commits into
masterfrom
feat/hosm

Conversation

@smilkuri

@smilkuri smilkuri commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Description

  • This PR updates CredentialProvider::memoize() to implement the shared credential refresh lifecycle. When a recoverable refresh fails, the SDK keeps using the last successful credentials and waits before contacting the credential source again. This behavior improves resilience to temporary credential source outages by allowing requests to continue reaching the service instead of failing immediately during client-side credential refresh.

  • The PR also calculates refresh timing from credential lifetime and classifies provider errors that require customer action as non-recoverable.

Behavior changes

  • The SDK now uses two refresh windows, advisory window of 5, 15, or 60 minutes based on credential lifetime.
    and mandatory window of 60 seconds.
  • Login credential provider refresh window is updated from 3min to 5min.
  • AssumeRoleWithWebIdentity no longer retries InvalidIdentityToken within the provider.
  • IMDS no longer extends credential expiration after a failed refresh. The shared lifecycle manages static stability.
  • When a CacheInterface is configured, the public cache() wrapper returned stored credentials until they expired. Refreshes in the advisory and mandatory windows never reached the credential source, and expired creds were dropped. Both conflict with the spec, which requires refreshing inside those windows and never discarding cached credentials after a failed refresh.
  • The default chain now uses a private cacheCredentials() wrapper for in-scope providers. It reads stored credentials once to start the lifecycle, sends later refreshes to the credential source, and keeps the last successful credentials when a refresh fails. Process credentials and the public cache() method keep their existing behavior.

Not in Scope

  • The spec requires invalidating cached credentials and retrying the request when a service returns ExpiredToken or ExpiredTokenException. This is not implemented because memoize() keeps its state in a closure with no second operation for the request pipeline to call. Adding it would require memoize() to return an object and new middleware, which changes long-standing public behavior. Credentials are refreshed through the expiration-based windows instead.
  • Automatic lifecycle for directly configured providers. The spec applies the lifecycle to in-scope providers created outside the default chain. In this PR, a provider passed directly to a client for example 'credentials' => CredentialProvider::instanceProfile() must still be wrapped in CredentialProvider::memoize(), as the current docs describe. Automatic wrapping at the client boundary is not in scope for this.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@smilkuri
smilkuri force-pushed the feat/hosm branch 3 times, most recently from f096d60 to 86cc54c Compare November 4, 2026 03:16
@smilkuri
smilkuri marked this pull request as ready for review November 4, 2026 03:17
@smilkuri
smilkuri requested a review from a team as a code owner November 4, 2026 03:17
@stobrien89 stobrien89 changed the title [DRAFT]: Add resilient credential refresh behaviour Add resilient credential refresh behaviour Nov 9, 2026
Comment on lines +12 to +13
{
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{
}
interface NonRecoverableCredentialsExceptionInterface extends \Throwable {}

Comment thread src/Exception/NoCredentialsError.php Outdated
Comment on lines +11 to +13
class NoCredentialsError extends CredentialsException
{
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rename and format like this:

Suggested change
class NoCredentialsError extends CredentialsException
{
}
class NoCredentialsException extends CredentialsException {}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can these tests be moved into CredentialProviderTest?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally we like to keep test classes per each runtime class

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we move this and ScriptedSourceClosure into a Utils sub-namespace/directory?

Comment thread src/Credentials/CredentialProvider.php Outdated
$result = $provider();
}
// Computes the advisory window from a credential set's lifetime.
$computeAdvisoryWindow = static function (

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these helper closures are recreated on every temporary-credential lookup including the cached path- could we move the fast path before their construction? Where possible, let's make consolidate and extract some of these closures to private static methods

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants