diff --git a/gittensor/utils/github_api_tools.py b/gittensor/utils/github_api_tools.py index cd5c0e4..41a8eec 100644 --- a/gittensor/utils/github_api_tools.py +++ b/gittensor/utils/github_api_tools.py @@ -248,7 +248,10 @@ def get_github_account_age_days(token: str) -> Optional[int]: def get_pull_request_file_changes(repository: str, pr_number: int, token: str) -> Optional[List[FileChange]]: """ - Get the diff for a specific PR by repository name and PR number + Get the diff for a specific PR by repository name and PR number. + + Uses retry logic with exponential backoff for transient failures. + Args: repository (str): Repository in format 'owner/repo' pr_number (int): PR number @@ -256,21 +259,42 @@ def get_pull_request_file_changes(repository: str, pr_number: int, token: str) - Returns: List[FileChanges]: List object with file changes or None if error """ + max_attempts = 3 headers = make_headers(token) - try: - response = requests.get( - f'{BASE_GITHUB_API_URL}/repos/{repository}/pulls/{pr_number}/files', headers=headers, timeout=15 - ) - if response.status_code == 200: - file_diffs = response.json() - return [FileChange.from_github_response(pr_number, repository, file_diff) for file_diff in file_diffs] + last_error = None + for attempt in range(max_attempts): + try: + response = requests.get( + f'{BASE_GITHUB_API_URL}/repos/{repository}/pulls/{pr_number}/files', headers=headers, timeout=15 + ) + if response.status_code == 200: + file_diffs = response.json() + return [FileChange.from_github_response(pr_number, repository, file_diff) for file_diff in file_diffs] - return [] + last_error = f'status {response.status_code}' + if attempt < max_attempts: + backoff_delay = min(5 * (2**attempt), 30) + bt.logging.warning( + f'File changes request for PR #{pr_number} in {repository} failed with status {response.status_code} ' + f'(attempt {attempt + 1}/{max_attempts}), retrying in {backoff_delay}s...' + ) + time.sleep(backoff_delay) - except Exception as e: - bt.logging.error(f'Error getting file changes for PR #{pr_number} in {repository}: {e}') - return [] + except requests.exceptions.RequestException as e: + last_error = str(e) + if attempt < max_attempts: + backoff_delay = min(5 * (2**attempt), 30) + bt.logging.warning( + f'File changes request error for PR #{pr_number} in {repository} ' + f'(attempt {attempt + 1}/{max_attempts}): {e}, retrying in {backoff_delay}s...' + ) + time.sleep(backoff_delay) + + bt.logging.error( + f'File changes request for PR #{pr_number} in {repository} failed after {max_attempts} attempts: {last_error}' + ) + return [] def execute_graphql_query( @@ -354,13 +378,14 @@ def get_github_graphql_query( max_attempts = 8 headers = {'Authorization': f'Bearer {token}', 'Content-Type': 'application/json'} - variables = { - 'userId': global_user_id, - 'limit': min(100, max_prs - merged_pr_count), - 'cursor': cursor, - } + limit = min(100, max_prs - merged_pr_count) for attempt in range(max_attempts): + variables = { + 'userId': global_user_id, + 'limit': limit, + 'cursor': cursor, + } try: response = requests.post( f'{BASE_GITHUB_API_URL}/graphql', @@ -375,9 +400,18 @@ def get_github_graphql_query( # error - log and retry if attempt < (max_attempts - 1): backoff_delay = min(5 * (2**attempt), 30) # cap at 30s - bt.logging.warning( - f'GraphQL request for PRs failed with status {response.status_code} (attempt {attempt + 1}/{max_attempts}), retrying in {backoff_delay}s...' - ) + # Reduce page size on server-side errors (query may be too expensive) + if response.status_code in (502, 503, 504): + limit = max(limit // 2, 10) + bt.logging.warning( + f'GraphQL request for PRs failed with status {response.status_code} ' + f'(attempt {attempt + 1}/{max_attempts}), page size set to {limit}, retrying in {backoff_delay}s...' + ) + else: + bt.logging.warning( + f'GraphQL request for PRs failed with status {response.status_code} ' + f'(attempt {attempt + 1}/{max_attempts}), retrying in {backoff_delay}s...' + ) time.sleep(backoff_delay) else: bt.logging.error( diff --git a/tests/utils/test_github_api_tools.py b/tests/utils/test_github_api_tools.py index beac169..9f7bf01 100644 --- a/tests/utils/test_github_api_tools.py +++ b/tests/utils/test_github_api_tools.py @@ -27,6 +27,7 @@ get_github_graphql_query = github_api_tools.get_github_graphql_query get_github_id = github_api_tools.get_github_id get_github_account_age_days = github_api_tools.get_github_account_age_days +get_pull_request_file_changes = github_api_tools.get_pull_request_file_changes # ============================================================================ @@ -101,6 +102,94 @@ def test_retry_on_502_then_success(self, mock_logging, mock_sleep, mock_post, gr # Verify exponential backoff: 5s, 10s mock_sleep.assert_has_calls([call(5), call(10)]) + @patch('gittensor.utils.github_api_tools.requests.post') + @patch('gittensor.utils.github_api_tools.time.sleep') + @patch('gittensor.utils.github_api_tools.bt.logging') + def test_502_halves_page_size_on_retry(self, mock_logging, mock_sleep, mock_post, graphql_params): + """Test that 502 errors cause the page size (limit) to be halved on each retry.""" + mock_response_502 = Mock(status_code=502, text='502 Bad Gateway') + mock_response_200 = Mock(status_code=200) + + mock_post.side_effect = [mock_response_502, mock_response_502, mock_response_200] + + result = get_github_graphql_query(**graphql_params) + + assert result.status_code == 200 + # Initial limit=100, halved to 50 after first 502, halved to 25 after second 502 + limits = [c.kwargs['json']['variables']['limit'] for c in mock_post.call_args_list] + assert limits == [100, 50, 25] + + @patch('gittensor.utils.github_api_tools.requests.post') + @patch('gittensor.utils.github_api_tools.time.sleep') + @patch('gittensor.utils.github_api_tools.bt.logging') + def test_page_size_floors_at_10(self, mock_logging, mock_sleep, mock_post, graphql_params): + """Test that page size never drops below 10 even after many 502s.""" + mock_response_502 = Mock(status_code=502, text='502 Bad Gateway') + mock_post.return_value = mock_response_502 + + get_github_graphql_query(**graphql_params) + + # 100 -> 50 -> 25 -> 12 -> 10 -> 10 -> 10 -> 10 + limits = [c.kwargs['json']['variables']['limit'] for c in mock_post.call_args_list] + assert limits == [100, 50, 25, 12, 10, 10, 10, 10] + + @patch('gittensor.utils.github_api_tools.requests.post') + @patch('gittensor.utils.github_api_tools.time.sleep') + @patch('gittensor.utils.github_api_tools.bt.logging') + def test_non_5xx_does_not_reduce_page_size(self, mock_logging, mock_sleep, mock_post, graphql_params): + """Test that non-5xx errors (e.g. 401) do not reduce page size.""" + mock_response_401 = Mock(status_code=401, text='Unauthorized') + mock_response_200 = Mock(status_code=200) + + mock_post.side_effect = [mock_response_401, mock_response_401, mock_response_200] + + result = get_github_graphql_query(**graphql_params) + + assert result.status_code == 200 + limits = [c.kwargs['json']['variables']['limit'] for c in mock_post.call_args_list] + assert limits == [100, 100, 100], 'Page size should stay at 100 for non-5xx errors' + + @patch('gittensor.utils.github_api_tools.requests.post') + @patch('gittensor.utils.github_api_tools.time.sleep') + @patch('gittensor.utils.github_api_tools.bt.logging') + def test_503_and_504_also_reduce_page_size(self, mock_logging, mock_sleep, mock_post, graphql_params): + """Test that 503 and 504 errors also trigger page size reduction.""" + mock_response_503 = Mock(status_code=503, text='Service Unavailable') + mock_response_504 = Mock(status_code=504, text='Gateway Timeout') + mock_response_200 = Mock(status_code=200) + + mock_post.side_effect = [mock_response_503, mock_response_504, mock_response_200] + + result = get_github_graphql_query(**graphql_params) + + assert result.status_code == 200 + limits = [c.kwargs['json']['variables']['limit'] for c in mock_post.call_args_list] + assert limits == [100, 50, 25] + + @patch('gittensor.utils.github_api_tools.requests.post') + @patch('gittensor.utils.github_api_tools.time.sleep') + @patch('gittensor.utils.github_api_tools.bt.logging') + def test_small_initial_limit_not_reduced_below_10(self, mock_logging, mock_sleep, mock_post): + """Test that a small initial limit (e.g. 15) floors correctly at 10.""" + mock_response_502 = Mock(status_code=502, text='502 Bad Gateway') + mock_response_200 = Mock(status_code=200) + + mock_post.side_effect = [mock_response_502, mock_response_502, mock_response_200] + + params = { + 'token': 'fake_github_token', + 'global_user_id': 'MDQ6VXNlcjEyMzQ1', + 'merged_pr_count': 85, + 'max_prs': 100, + 'cursor': None, + } + result = get_github_graphql_query(**params) + + assert result.status_code == 200 + # Initial limit = min(100, 100-85) = 15, halved to 10, stays at 10 + limits = [c.kwargs['json']['variables']['limit'] for c in mock_post.call_args_list] + assert limits == [15, 10, 10] + @patch('gittensor.utils.github_api_tools.requests.post') @patch('gittensor.utils.github_api_tools.time.sleep') @patch('gittensor.utils.github_api_tools.bt.logging') @@ -292,5 +381,116 @@ def test_get_github_account_age_retry_logic(self, mock_sleep, mock_get, clear_gi assert mock_get.call_count == 2 +# ============================================================================ +# File Changes Retry Logic Tests +# ============================================================================ + + +class TestFileChangesRetryLogic: + """Test suite for retry logic in get_pull_request_file_changes.""" + + @patch('gittensor.utils.github_api_tools.requests.get') + def test_successful_request_no_retry(self, mock_get): + """Test that a successful request returns file changes without retrying.""" + mock_response = Mock(status_code=200) + mock_response.json.return_value = [ + { + 'filename': 'test.py', + 'status': 'modified', + 'changes': 2, + 'additions': 1, + 'deletions': 1, + 'patch': '@@ -1 +1 @@\n-old\n+new', + }, + ] + mock_get.return_value = mock_response + + result = get_pull_request_file_changes('owner/repo', 1, 'fake_token') + + assert mock_get.call_count == 1 + assert result is not None + assert len(result) == 1 + + @patch('gittensor.utils.github_api_tools.requests.get') + @patch('gittensor.utils.github_api_tools.time.sleep') + @patch('gittensor.utils.github_api_tools.bt.logging') + def test_retry_on_502_then_success(self, mock_logging, mock_sleep, mock_get): + """Test that 502 triggers retry and succeeds on second attempt.""" + mock_502 = Mock(status_code=502, text='Bad Gateway') + mock_200 = Mock(status_code=200) + mock_200.json.return_value = [ + {'filename': 'test.py', 'status': 'modified', 'changes': 1, 'additions': 1, 'deletions': 0, 'patch': ''}, + ] + + mock_get.side_effect = [mock_502, mock_200] + + result = get_pull_request_file_changes('owner/repo', 1, 'fake_token') + + assert mock_get.call_count == 2 + assert mock_sleep.call_count == 1 + assert len(result) == 1 + + @patch('gittensor.utils.github_api_tools.requests.get') + @patch('gittensor.utils.github_api_tools.time.sleep') + @patch('gittensor.utils.github_api_tools.bt.logging') + def test_gives_up_after_three_attempts(self, mock_logging, mock_sleep, mock_get): + """Test that function gives up after 3 failed attempts and returns empty list.""" + mock_500 = Mock(status_code=500, text='Internal Server Error') + mock_get.return_value = mock_500 + + result = get_pull_request_file_changes('owner/repo', 1, 'fake_token') + + assert mock_get.call_count == 3 + assert mock_sleep.call_count == 3 + assert result == [] + mock_logging.error.assert_called() + + @patch('gittensor.utils.github_api_tools.requests.get') + @patch('gittensor.utils.github_api_tools.time.sleep') + @patch('gittensor.utils.github_api_tools.bt.logging') + def test_retry_on_connection_error_then_success(self, mock_logging, mock_sleep, mock_get): + """Test that connection errors trigger retry.""" + import requests + + mock_200 = Mock(status_code=200) + mock_200.json.return_value = [] + + mock_get.side_effect = [requests.exceptions.ConnectionError('refused'), mock_200] + + result = get_pull_request_file_changes('owner/repo', 1, 'fake_token') + + assert mock_get.call_count == 2 + assert mock_sleep.call_count == 1 + assert result == [] + + @patch('gittensor.utils.github_api_tools.requests.get') + @patch('gittensor.utils.github_api_tools.time.sleep') + @patch('gittensor.utils.github_api_tools.bt.logging') + def test_gives_up_after_three_connection_errors(self, mock_logging, mock_sleep, mock_get): + """Test that function gives up after 3 connection errors.""" + import requests + + mock_get.side_effect = requests.exceptions.ConnectionError('refused') + + result = get_pull_request_file_changes('owner/repo', 1, 'fake_token') + + assert mock_get.call_count == 3 + assert result == [] + mock_logging.error.assert_called() + + @patch('gittensor.utils.github_api_tools.requests.get') + @patch('gittensor.utils.github_api_tools.time.sleep') + @patch('gittensor.utils.github_api_tools.bt.logging') + def test_exponential_backoff_timing(self, mock_logging, mock_sleep, mock_get): + """Test that backoff delays are 5s, 10s for 3 attempts.""" + mock_500 = Mock(status_code=500, text='Internal Server Error') + mock_get.return_value = mock_500 + + get_pull_request_file_changes('owner/repo', 1, 'fake_token') + + mock_sleep.assert_has_calls([call(5), call(10), call(20)]) + assert mock_sleep.call_count == 3 + + if __name__ == '__main__': pytest.main([__file__, '-v'])