Skip to content

Conversation

@konstin
Copy link
Member

@konstin konstin commented Dec 12, 2025

Previously, we dropped the counts from the middleware layer, potentially doing to many retries and/or reporting too few.

Not pretty but fixes the bug.

@konstin konstin temporarily deployed to uv-test-registries December 12, 2025 17:02 — with GitHub Actions Inactive
@zanieb zanieb requested a review from EliteTK December 12, 2025 18:18
@konstin konstin force-pushed the konsti/better-retry-handling-3 branch from 1e18331 to 7571ffa Compare December 15, 2025 13:31
@konstin konstin force-pushed the konsti/better-retry-handling-4 branch from 75c964a to 1faec8b Compare December 15, 2025 13:32
@konstin konstin force-pushed the konsti/better-retry-handling-4 branch from 1faec8b to f4a3fac Compare December 15, 2025 13:45
Client(Error),
/// Track retries before a callback explicitly, as we can't attach them to the callback error
/// type.
Callback { retries: u32, err: CallbackError },
Copy link
Member Author

@konstin konstin Dec 15, 2025

Choose a reason for hiding this comment

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

Removed the optional as zero retries is indistinguishable from None.

@konstin konstin added the bug Something isn't working label Dec 15, 2025
@konstin konstin marked this pull request as ready for review December 15, 2025 13:46
@konstin konstin temporarily deployed to uv-test-registries December 15, 2025 15:21 — with GitHub Actions Inactive
Copy link
Contributor

@EliteTK EliteTK left a comment

Choose a reason for hiding this comment

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

I'm a little bit too unfamiliar with the underlying code to understand why you consider this to be a hack but I've given it a thorough looking over and I can't see anything that stands out.

The surrounding code may benefit from some future rework, but I haven't looked deeply into it here as it's not really related to this specific PR.

Err(err) => {
// Check if the middleware already performed retries
total_retries += err.retries().unwrap_or_default();
if is_transient_network_error(err.error()) {
// If middleware already retried, consider that in our retry budget
let retry_decision = retry_policy.should_retry(start_time, total_retries);
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: I think the second comment doesn't make much sense now the addition (consideration) has moved.

Suggested change
Err(err) => {
// Check if the middleware already performed retries
total_retries += err.retries().unwrap_or_default();
if is_transient_network_error(err.error()) {
// If middleware already retried, consider that in our retry budget
let retry_decision = retry_policy.should_retry(start_time, total_retries);
Err(err) => {
// Check if the middleware already performed retries and consider it in our retry budget
total_retries += err.retries().unwrap_or_default();
if is_transient_network_error(err.error()) {
let retry_decision = retry_policy.should_retry(start_time, total_retries);

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Will look at it tomorrow.

Comment on lines +730 to +734
return if total_retries > 0 {
Err(err.with_retries(total_retries))
} else {
Err(err)
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return if total_retries > 0 {
Err(err.with_retries(total_retries))
} else {
Err(err)
};
return Err(err.with_retries(total_retries));

Since the retry count is non-optional, conditionally doing with_retries here is now somewhat meaningless.

Base automatically changed from konsti/better-retry-handling-3 to main December 18, 2025 10:10
Previously, we dropped the counts from the middleware layer, potentially doing to many retries and/or reporting too few.

Not pretty but fixes the bug.
@konstin konstin force-pushed the konsti/better-retry-handling-4 branch from f4a3fac to 24e22f2 Compare December 18, 2025 10:11
@konstin konstin enabled auto-merge (squash) December 18, 2025 10:11
@konstin konstin temporarily deployed to uv-test-registries December 18, 2025 10:14 — with GitHub Actions Inactive
@konstin konstin merged commit a25d4f9 into main Dec 18, 2025
187 of 192 checks passed
@konstin konstin deleted the konsti/better-retry-handling-4 branch December 18, 2025 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants