Skip to content

Join errors when retry token is unavailable #3121

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

NetcraftPm
Copy link

If we fail to acquire a retry token, we are currently discarding the original error. According to the Retry Behaviour reference guide, we are expected to return the actual response (error) in this situation.

With these changes, we now return the joined error, which allows callers to inspect and match on the error that caused the request to fail.

@NetcraftPm NetcraftPm requested a review from a team as a code owner June 25, 2025 16:43
If we fail to acquire a retry token, we are currently discarding the original
error. According to the Retry Behaviour reference guide, we are expected to
return the actual response (error) in this situation.

With these changes, we now return the joined error, which allows callers to
inspect and match on the error that caused the request to fail.
@lucix-aws
Copy link
Contributor

This seems ok but shouldn't the retry error wrap the original error instead?

@NetcraftPm
Copy link
Author

This seems ok but shouldn't the retry error wrap the original error instead?

I'm happy to make it wrap the error in GetRetryToken instead if you prefer. My logic was that it seemed that given the method name is GetRetryToken it doesn't make much sense for it to wrap the error it's given, as that's conceptually to determine what (size of) token to reserve. We could also use fmt.Errorf to join the errors, but it feels like that would make the actual error string quite hard to read.

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