Repository navigation
Conversation
smilkuri
force-pushed
the
feat/hosm
branch
3 times, most recently
from
November 4, 2026 03:16
f096d60 to
86cc54c
Compare
smilkuri
marked this pull request as ready for review
November 4, 2026 03:17
stobrien89
requested changes
Nov 9, 2026
Comment on lines
+12
to
+13
| { | ||
| } |
Member
There was a problem hiding this comment.
Suggested change
| { | |
| } | |
| interface NonRecoverableCredentialsExceptionInterface extends \Throwable {} |
Comment on lines
+11
to
+13
| class NoCredentialsError extends CredentialsException | ||
| { | ||
| } |
Member
There was a problem hiding this comment.
Can we rename and format like this:
Suggested change
| class NoCredentialsError extends CredentialsException | |
| { | |
| } | |
| class NoCredentialsException extends CredentialsException {} |
Member
There was a problem hiding this comment.
Can these tests be moved into CredentialProviderTest?
Member
There was a problem hiding this comment.
generally we like to keep test classes per each runtime class
Member
There was a problem hiding this comment.
can we move this and ScriptedSourceClosure into a Utils sub-namespace/directory?
| $result = $provider(); | ||
| } | ||
| // Computes the advisory window from a credential set's lifetime. | ||
| $computeAdvisoryWindow = static function ( |
Member
There was a problem hiding this comment.
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
and mandatory window of 60 seconds.
InvalidIdentityTokenwithin the provider.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
ExpiredTokenorExpiredTokenException. This is not implemented becausememoize()keeps its state in a closure with no second operation for the request pipeline to call. Adding it would requirememoize()to return an object and new middleware, which changes long-standing public behavior. Credentials are refreshed through the expiration-based windows instead.'credentials' => CredentialProvider::instanceProfile()must still be wrapped inCredentialProvider::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.