Repository navigation
fix(services/s3): keep retryable sign errors temporary - #8458
Open
DimaRabkin wants to merge 1 commit into
Open
DimaRabkin wants to merge 1 commit into
DimaRabkin wants to merge 1 commit into
Conversation
What: For general-purpose buckets, S3 maps a retryable reqsign error to a temporary OpenDAL error, as the S3 Express path already does. Why: Every signing failure became a persistent Unexpected error, so RetryLayer never retried a transient credential failure such as a failed STS AssumeRole send. How: S3Core::sign_error passes is_retryable() through with_temporary. S3Builder::build delegates to build_backend so tests can replace the signer. Tests cover a provider that fails once with a retryable error and then succeeds, and a persistent error that stays persistent.
github-actions
Bot
added
releases-note/fix
This branch has not been deployed
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.
Which issue does this PR close?
Closes #8457.
Rationale for this change
For general-purpose buckets, S3 reports every signing failure as a persistent
Unexpectederror, even when reqsign marks the failure retryable. A transient credential failure, such as a failed STS AssumeRole send, is therefore never retried byRetryLayer. The S3 Express path already keeps the flag.Today: a retryable reqsign error becomes a persistent "signing http request" error.
After this PR: a retryable reqsign error becomes a temporary error, so
RetryLayerretries it. A persistent reqsign error stays persistent. The error kind and message are unchanged.This PR fixes the OpenDAL half. Today
ProvideCredentialChainin reqsign drops retryable provider errors before they reach OpenDAL; apache/reqsign#926 fixes that. S3 always wraps its providers in a chain, so the retry also needs that fix. It ships as a semver-compatiblereqsign-core3.3.x release, so no dependency change is needed here.What changes are included in this PR?
S3Core::sign_errorpassesis_retryable()through aswith_temporaryfor general-purpose buckets, as it already does for S3 Express.S3Builder::builddelegates to an inherentbuild_backendthat returns the concreteS3Backend, so tests can replace the signer.test_s3_retryable_credential_error_is_temporary: a credential provider fails once with a retryable error and then succeeds. The firststatfails with a temporary error before any request is sent; the retry signs with the loaded credential. It fails onmain(is_temporary()is false).test_s3_persistent_credential_error_is_not_temporary: a persistent credential error stays persistent.The tests install the provider directly on the signer, not inside a
ProvideCredentialChain, because the chain drops the retryable error until the reqsign fix is released.Are there any user-facing changes?
Retryable signing failures on general-purpose S3 buckets are now temporary errors, so
RetryLayerretries them.Breaking changes
AI Usage Statement
claude-opus-5-5)RetryLayer, wrote the change and tests, and drafted this description. The failure was observed in production against AWS STS (one transient AssumeRole send failure); the tests simulate the provider failure.