Skip to content

fix: #341 align attempt retry count with await and recover#348

Open
edmoffo wants to merge 1 commit into
zerocracy:masterfrom
edmoffo:341-attempt-retry-count
Open

fix: #341 align attempt retry count with await and recover#348
edmoffo wants to merge 1 commit into
zerocracy:masterfrom
edmoffo:341-attempt-retry-count

Conversation

@edmoffo

@edmoffo edmoffo commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Issue #341 noticed that BazaRb#attempt passed @retries to with_retries(max_tries:) while #await and #recover treated @retries as a retry count, so a retries: 5 configuration produced four TimedOut retries against five 429 and >=499 retries. The YARD on lib/baza-rb.rb:58 names the kwarg "Number of retries on connection failure", which matches the loop semantics in await and recover and contradicts the with_retries semantics in attempt.

The diff bumps max_tries: in attempt to @retries + 1 so all three retry layers interpret the constructor's retries: kwarg as the same retry count. The visible effect is that a BazaRb.new(host, port, token, retries: N) now performs N + 1 total tries on TimedOut, 429, and >=499 — one initial call and N retries on each path. The new test_attempt_total_tries_match_retries_plus_one in test/test_baza_edge.rb pins the count at @retries + 1 total tries on a persistent TimedOut.

Local checks on Ruby 3.3.6: bundle exec ruby -Ilib -Itest test/test_baza_edge.rb -n test_attempt_total_tries_match_retries_plus_one -> 1 test, 2 assertions, 0 failures; bundle exec ruby -Ilib -Itest test/test_baza_edge.rb -n '/retries|retry|attempt|partial_file/' -> 8 tests, 21 assertions, 0 failures; bundle exec ruby -Ilib -Itest test/test_baza_edge.rb -> 39 tests, 1 failure and 7 errors all reproducing on origin/master (test_durable_load_in_chunks, test_real_http, the four test_push_with_*/test_push_compress* cases, test_with_very_short_timeout, test_durable_load_from_sinatra) and unrelated to this diff; bundle exec rubocop lib/baza-rb.rb test/test_baza_edge.rb -> no offenses; bundle exec yard --fail-on-warning 'lib/**/*.rb' -> 96.23% documented, no warnings.

Closes #341.

with_retries treats max_tries as the total attempt count, so passing
@retries gave @retries - 1 retries on TimedOut while #await and #recover
gave @retries retries on 429 and >=499. Pass @retries + 1 to with_retries
so the three retry layers all interpret the constructor's retries: kwarg
as the retry count, matching the YARD on line 58.

A new test_attempt_total_tries_match_retries_plus_one in
test/test_baza_edge.rb pins the count at @retries + 1 total tries on a
persistent TimedOut.

@GHX5T-SOL GHX5T-SOL left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved. This current head fixes the retry-count mismatch from #341 by treating @retries as a retry count consistently: attempt now passes max_tries: @retries + 1, matching the existing await/recover loop semantics and the constructor YARD. The added test_attempt_total_tries_match_retries_plus_one pins the timeout path at one initial try plus @retries retries, so the one-line source change is covered.

Validation I ran on exact head ddfc2c9e098a75081ade1ce534e5fe04184cd291:

  • bundle check -> dependencies satisfied
  • ruby -c lib/baza-rb.rb -> Syntax OK
  • git diff --check upstream/master...HEAD -> passed
  • focused test_attempt_total_tries_match_retries_plus_one with coverage disabled -> 1 test, 2 assertions, 0 failures
  • retry/attempt subset with coverage disabled (/retries|retry|attempt|partial_file/) -> 8 tests, 21 assertions, 0 failures
  • full test/test_baza_edge.rb with coverage disabled -> 39 tests, 110 assertions, 0 failures
  • full test/test_fake.rb with coverage disabled -> 36 tests, 46 assertions, 0 failures
  • bundle exec rubocop -> 12 files inspected, no offenses
  • bundle exec rake picks -> passed
  • bundle exec rake yard -> 96.23% documented

Hosted readback: the touched regression passes on both hosted rake logs. Ubuntu reached 109 tests, 200 assertions, 0 failures, 0 errors before the workflow was canceled after style/docs completion. macOS has one failure, but it is the known live TestBazaLive#test_live_full_cycle stuck-job case (Job #74847 ... did not finish) while the retry-count regression and other relevant local tests pass. I did not run local full bundle exec rake because it invokes live Zerocracy API tests, which I kept out of scope.

@0crat

0crat commented Jun 10, 2026

Copy link
Copy Markdown

@GHX5T-SOL Hey! Nice work on the review! 🎉 You scored +6 points this time: started with the standard +18 base, but lost -8 for zero comments (our policy really values reviewer feedback!) and -4 for having only 19 hits-of-code (we need at least 24). Your running total is now +1372 - keep it up and don't forget to check your Zerocracy account for updates!

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.

BazaRb#attempt treats @retries as max_tries while #await/#recover treat it as retry count, so timeout retries are one short of 429/5xx retries

3 participants