Skip to content

fix: resolve retry logic inconsistencies in github_api_tools#195

Merged
anderdc merged 2 commits intoentrius:testfrom
eren-karakus0:fix/retry-logic-inconsistencies
Feb 26, 2026
Merged

fix: resolve retry logic inconsistencies in github_api_tools#195
anderdc merged 2 commits intoentrius:testfrom
eren-karakus0:fix/retry-logic-inconsistencies

Conversation

@eren-karakus0
Copy link
Contributor

Summary

This PR fixes 5 related bugs in gittensor/utils/github_api_tools.py, all involving retry logic, type annotations, and log message accuracy.

Changes

1. Off-by-one in get_pull_request_file_changes() retry guard (lines 278, 288)

Bug: The condition attempt < max_attempts is always True inside a range(max_attempts) loop, causing an unnecessary time.sleep() after the last failed attempt before the function returns. This adds up to 20 seconds of wasted delay on total failure.

Fix: Changed to attempt < (max_attempts - 1) to match the pattern used in execute_graphql_query() and get_github_graphql_query().

2. Uncapped exponential backoff in execute_graphql_query() exception handler (line 352)

Bug: The HTTP status code retry path correctly caps backoff at 30s with min(5 * (2**attempt), 30), but the RequestException handler computes backoff_delay = 5 * (2**attempt) without a cap. At attempt 6, this produces a 320-second (5+ minute) sleep.

Fix: Added min(..., 30) cap to match the status code path and other retry functions.

3. Incorrect docstring default value in execute_graphql_query() (line 316)

Bug: Docstring says max_attempts: Maximum retry attempts (default 6) but the actual function signature is max_attempts: int = 8.

Fix: Updated docstring to (default 8).

4. Wrong type annotation in load_miners_prs() (line 595)

Bug: prs: Dict = pr_data.get('nodes', []) — the nodes key returns a JSON array (Python list), not a dict.

Fix: Changed annotation to prs: List.

5. Misleading log message in should_skip_merged_pr() (line 498)

Bug: The condition merged_dt < lookback_date_filter filters PRs merged before the lookback window, but the log says "merged within {PR_LOOKBACK_DAYS} day lookback window", implying the opposite.

Fix: Changed to "merged before {PR_LOOKBACK_DAYS}-day lookback window".

Test Evidence

Updated and added tests in tests/utils/test_github_api_tools.py:

  • Updated TestFileChangesRetryLogic.test_gives_up_after_three_attempts: sleep count assertion corrected from 3 to 2 (no sleep on last attempt)
  • Updated TestFileChangesRetryLogic.test_exponential_backoff_timing: expected delays corrected from [5, 10, 20] to [5, 10]
  • Added TestExecuteGraphQLQueryRetryLogic class (3 tests):
    • test_exception_backoff_capped_at_30s — verifies backoff cap in exception handler
    • test_status_code_backoff_capped_at_30s — verifies backoff cap in status code handler
    • test_successful_request_no_retry — verifies no retry on success
  • Added TestShouldSkipMergedPr class (2 tests):
    • test_log_message_says_before_for_old_pr — verifies corrected log wording
    • test_recent_pr_not_skipped_by_lookback — verifies recent PRs pass lookback check

All existing tests continue to pass. Ruff linting passes with no warnings.

$ ruff check gittensor/utils/github_api_tools.py tests/utils/test_github_api_tools.py
All checks passed!

Copy link
Collaborator

@anderdc anderdc left a comment

Choose a reason for hiding this comment

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

I commented on what I like, everything else, reset/revert it

@eren-karakus0
Copy link
Contributor Author

eren-karakus0 commented Feb 20, 2026

Done, reverted all changes except the two you approved:

  1. execute_graphql_query backoff cap (min(5 * (2**attempt), 30))
  2. prs: List type annotation fix

Kept the corresponding tests for these two changes, reverted everything else including the get_pull_request_file_changes retry boundary change and max_attempts docstring.

@anderdc
Copy link
Collaborator

anderdc commented Feb 23, 2026

sorry took a while to get back to this, resolve conflicts then it'll be good to merge thanks

@eren-karakus0 eren-karakus0 force-pushed the fix/retry-logic-inconsistencies branch from 6c93938 to e41a68c Compare February 24, 2026 08:02
@eren-karakus0
Copy link
Contributor Author

Rebased on latest test branch - conflicts resolved. Ready to merge.

@anderdc
Copy link
Collaborator

anderdc commented Feb 26, 2026

hmm another conflict, resolve so I can get this in today

I was doing reviews on other PRs so that caused a conflict here

- Fix off-by-one error in get_pull_request_file_changes() that caused an
  unnecessary sleep after the final attempt (attempt < max_attempts should
  be attempt < max_attempts - 1, consistent with other retry functions)
- Cap exponential backoff at 30s in execute_graphql_query() exception
  handler (status code path was already capped, exception path was not)
- Fix docstring in execute_graphql_query() stating default max_attempts
  is 6 when the actual default parameter value is 8
- Fix type annotation in load_miners_prs(): prs variable annotated as
  Dict but actually receives a List from .get('nodes', [])
- Fix misleading log in should_skip_merged_pr(): message said "merged
  within lookback window" but the condition filters PRs merged BEFORE
  the lookback window
…pe fix

Revert get_pull_request_file_changes retry boundary change and
max_attempts docstring change per reviewer request. Keep approved
changes: backoff cap in execute_graphql_query and prs type annotation.
@eren-karakus0 eren-karakus0 force-pushed the fix/retry-logic-inconsistencies branch from e5e90a4 to 8dd03d4 Compare February 26, 2026 17:58
@eren-karakus0
Copy link
Contributor Author

Rebased on latest test branch - conflicts resolved. Ready to merge.

Copy link
Collaborator

@anderdc anderdc left a comment

Choose a reason for hiding this comment

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

LGTM

@anderdc anderdc merged commit b9ce097 into entrius:test Feb 26, 2026
2 checks passed
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