[alextwoods]

Note - This branch merges to the feature/master/credential_cache feature branch and NOT to master.

Motivation and Context

Based on updates in the spec, we want to retry credential invalidating exceptions so that they will (hopefully) succeed on later attempts (after new credentials have been resolved).

Modifications

There are two changes as part of this:

1. Make ExpiredToken and InvalidToken retryable. This is pretty straightforward - these errors will now just get retried like normal.
2. Re-Resolve identity (aka, credentilas) on each retry attempt (that is, every request attempt after the first). This is new behavior for the Java SDK, but is standard for SRA-compliant SDKs. See the Credential resolution / request lifecycle section below for more.....

Credential resolution / request lifecycle

Credentials (Identity) are resolved in the AuthSchemeResolutionStage when the auth scheme is resovled. This happens BEFORE the retry stage and is used in multiple places before retry:

1. ApplyUserAgentStage (for credential specific business metrics)
2. EndpointResolutionStage (to get the accountId)

So, without larger changes to our pipeline, we can't push initial identity resolution into the retry loop. Instead, we re-resolve that identity during the loop (for non-initial attempts) using the identityProvider/options that were resolved earlier.

Testing

New and existing tests, including E2E spec tests

Screenshots (if appropriate)

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)

License

• I confirm that this pull request can be released under the Apache 2 license

[joviegas]

On async clients, retries reach this line through scheduleOrFail (line 187), so every retry resolves identity on an sdk-ScheduledExecutor thread.
For the SDK credential providers, resolveIdentity calls
resolveCredentials() on the calling thread. After an ExpiredToken, invalidate() marks the CachedSupplier value stale, so the retry's get() does a blocking refresh (an STS, IMDS, or container call). Other threads can wait on the refresh lock for up to 5 seconds(BLOCKING_REFRESH_MAX_WAIT) .

The default scheduler has 5 threads. It also runs the apiCallTimeout timers, the apiCallAttemptTimeout timers, and the retry backoff timers for every request on the client. If 5 or more requests retry an ExpiredToken at the same time, will the timeouts and retries of unrelated requests fire late until the refresh completes?
Can we check this scenario: 5 async calls get ExpiredToken while a 6th call with apiCallTimeout(300ms) is in flight?
WDYT?

[alextwoods]

Yeah - this is a good callout. I was struggling with where/how to run the refresh on the async path. I went with the scheduler thread since it was already doing things like signing... but you are correct that a blocked/slow refresh would impact the api call timeout.

I've moved all of the retry execution into the futureCompletionExecutor which I think is the more correct place for all of that retry loop logic to live. That fixes the issue with the apiCallTimeout (but does cost one more thread jump, but I think its logically more correct for all of the retry loop to execute outside of the scheduler threads).

[joviegas]

What happens when these two codes are retried on these paths?

1. A legacy signer override (SdkAdvancedClientOption.SIGNER)
2. An interceptor that sets AWS_CREDENTIALS
3. S3 presignedUrlExtension().getObject

On paths 1 and 2, AwsRequestIdentityProviderResolver (line 68) replaces the provider with StaticCredentialsProvider, whose invalidate() is a no-op. On path 3, the credentials are in the URL. Since the retry attempt signs with the same credentials (or sends the same URL), will these calls make up to 3 extra attempts with the default retry mode and still end with ExpiredToken, where before this change they failed after 1 attempt

WDYT about skipping the retry for these two codes when the identity can't change?

[alextwoods]

Yeah these are valid - each of these cases will result in new retries that are unlikely to succeed, but thats the cross-SDK spec for them.

I don't think we can reliably detect "identities that can't change" and the retry spec requires that we retry them unconditionally....

[github-actions]

This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one.