Skip to content

Fix retry delay going negative for large retries with exponential delays - #2164

Merged
martincostello merged 7 commits into
App-vNext:mainfrom
martincostello:gh-2163
Jun 25, 2024
Merged

martincostello merged 7 commits into
App-vNext:mainfrom
martincostello:gh-2163

Conversation

@martincostello

@martincostello martincostello commented Jun 25, 2024 •

Copy link
Copy Markdown
Member
  • Fix retry delays going negative with DelayBackoffType.Exponential after 1,024 retries.
  • Only compute the effective maximum TimeSpan once.
  • Assert delays are positive.
  • Suppress some xUnit1042 warnings.

Resolves #2163.

Suppress `xUnit1042` warnings.
Add an assertion for the delay's value not being negative.
- Fix retry delays going negative with `DelayBackoffType.Exponential` after 1,024 retries.
- Only compute the effective maximum `TimeSpan` once.
- Improve regression test.
- Assert delays are positive.
Resolves App-vNext#2163.
- Add more unit tests related to the exponential delay becoming negative.
- Remove redundant Stryker comment.
@martincostello martincostello added this to the v8.4.1 milestone Jun 25, 2024
@codecov

codecov Bot commented Jun 25, 2024 •

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.78%. Comparing base (5d81f99) to head (1300d7f).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2164   +/-   ##
=======================================
  Coverage   83.77%   83.78%           
=======================================
  Files         313      313           
  Lines        7148     7152    +4     
  Branches     1056     1057    +1     
=======================================
+ Hits         5988     5992    +4     
  Misses        789      789           
  Partials      371      371           
Flag Coverage Δ
linux 83.78% <100.00%> (+<0.01%) ⬆️
macos 83.78% <100.00%> (+<0.01%) ⬆️
windows ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Turns out it's the second part that's (now) unreachable.
Comment thread src/Polly.Core/Retry/RetryHelper.cs Outdated
Comment thread src/Polly.Core/Retry/RetryResilienceStrategy.cs
Comment thread test/Polly.Core.Tests/Issues/IssuesTests.InfiniteRetry_2163.cs
Comment thread test/Polly.Core.Tests/Issues/IssuesTests.InfiniteRetry_2163.cs Outdated
Comment thread test/Polly.Core.Tests/Retry/RetryHelperTests.cs Outdated
- Rename field.
- Remove redundant comment.
- Simplify repro slightly.
- Use `TheoryData<int>` for retry test cases.
@martincostello
martincostello enabled auto-merge (squash) June 25, 2024 11:53
@martincostello
martincostello merged commit 7301ef3 into App-vNext:main Jun 25, 2024
@martincostello
martincostello deleted the gh-2163 branch June 25, 2024 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: GetRetryDelay goes NEGATIVE after 1025 attempts [High Risk Issue]

2 participants