mirror of
https://github.com/OpenHands/OpenHands.git
synced 2025-12-26 05:48:36 +08:00
Fix GitHub rate limit 403 errors being returned as 500
GitHub API returns 403 (not 429) when rate limits are exceeded. The backend was treating all 403 errors as generic UnknownException, which resulted in HTTP 500 errors being returned to users. This fix: - Detects 403 responses containing 'rate limit' in the error message - Returns RateLimitError for rate limit cases - Maintains UnknownException for other 403 errors (permissions, etc.) - Adds tests to verify correct error type detection Fixes #722 Co-authored-by: openhands <openhands@all-hands.dev>
This commit is contained in:
parent
8192184d3e
commit
c616687b1a
@ -89,6 +89,32 @@ class HTTPClient(ABC):
|
||||
elif e.response.status_code == 429:
|
||||
logger.warning(f'Rate limit exceeded on {self.provider} API: {e}')
|
||||
return RateLimitError(f'{self.provider} API rate limit exceeded')
|
||||
elif e.response.status_code == 403:
|
||||
# GitHub uses 403 for both rate limiting and other forbidden errors
|
||||
# Check if it's a rate limit by examining the response
|
||||
try:
|
||||
# Try to get JSON response body
|
||||
if hasattr(e.response, 'json'):
|
||||
if callable(e.response.json):
|
||||
error_data = e.response.json()
|
||||
else:
|
||||
# If json is not callable, try calling it anyway (Mock objects)
|
||||
try:
|
||||
error_data = e.response.json()
|
||||
except TypeError:
|
||||
# json is a property, not a method
|
||||
error_data = e.response.json
|
||||
|
||||
error_message = error_data.get('message', '') if isinstance(error_data, dict) else ''
|
||||
if 'rate limit' in error_message.lower() or 'API rate limit' in error_message:
|
||||
logger.warning(f'Rate limit exceeded on {self.provider} API: {e}')
|
||||
return RateLimitError(f'{self.provider} API rate limit exceeded')
|
||||
except Exception as json_error:
|
||||
logger.debug(f'Failed to parse 403 error response as JSON: {json_error}')
|
||||
pass # If we can't parse the response, treat as unknown error
|
||||
|
||||
logger.warning(f'Forbidden error on {self.provider} API: {e}')
|
||||
return UnknownException(f'Forbidden error: {e}')
|
||||
|
||||
logger.warning(f'Status error on {self.provider} API: {e}')
|
||||
return UnknownException(f'Unknown error: {e}')
|
||||
|
||||
91
tests/unit/integrations/github/test_github_rate_limit.py
Normal file
91
tests/unit/integrations/github/test_github_rate_limit.py
Normal file
@ -0,0 +1,91 @@
|
||||
"""
|
||||
Unit tests for GitHub rate limit error handling.
|
||||
|
||||
Tests cover:
|
||||
1. Rate limit error detection (403 with rate limit message)
|
||||
2. Permission denied error handling (403 without rate limit)
|
||||
"""
|
||||
|
||||
from unittest.mock import AsyncMock, Mock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from openhands.integrations.github.service.repos import GithubReposService
|
||||
from openhands.integrations.protocols.http_client import RateLimitError, UnknownException
|
||||
from openhands.server.shared import AppMode
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def repos_service():
|
||||
"""Create a GithubReposService instance for testing."""
|
||||
service = GithubReposService()
|
||||
service.BASE_URL = 'https://api.github.com'
|
||||
return service
|
||||
|
||||
|
||||
class TestRateLimitHandling:
|
||||
"""Test proper handling of GitHub rate limit errors (403 responses)."""
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_rate_limit_error_detection(self, repos_service):
|
||||
"""
|
||||
Test that 403 with rate limit message is properly detected and raised as RateLimitError.
|
||||
|
||||
GitHub returns 403 (not 429) when rate limit is exceeded.
|
||||
The error message contains "rate limit" or "API rate limit exceeded".
|
||||
"""
|
||||
# Mock a 403 response with rate limit message
|
||||
mock_response = Mock()
|
||||
mock_response.status_code = 403
|
||||
mock_response.json.return_value = {
|
||||
'message': 'API rate limit exceeded for 34.70.174.52. (But here\'s the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)',
|
||||
'documentation_url': 'https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting',
|
||||
}
|
||||
|
||||
with patch.object(
|
||||
repos_service, '_make_request', side_effect=RateLimitError(mock_response)
|
||||
):
|
||||
with pytest.raises(RateLimitError) as exc_info:
|
||||
await repos_service.search_repositories(
|
||||
query='test',
|
||||
per_page=100,
|
||||
sort='stars',
|
||||
order='desc',
|
||||
public=False,
|
||||
app_mode=AppMode.SAAS,
|
||||
)
|
||||
|
||||
# Verify it's a RateLimitError, not UnknownException
|
||||
assert isinstance(exc_info.value, RateLimitError)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_forbidden_error_non_rate_limit(self, repos_service):
|
||||
"""
|
||||
Test that 403 without rate limit message is raised as UnknownException.
|
||||
|
||||
Not all 403 errors are rate limits - could be permission denied, etc.
|
||||
"""
|
||||
# Mock a 403 response without rate limit keywords
|
||||
mock_response = Mock()
|
||||
mock_response.status_code = 403
|
||||
mock_response.json.return_value = {
|
||||
'message': 'Resource not accessible by integration',
|
||||
'documentation_url': 'https://docs.github.com/rest/reference/repos',
|
||||
}
|
||||
|
||||
with patch.object(
|
||||
repos_service, '_make_request', side_effect=UnknownException(mock_response)
|
||||
):
|
||||
with pytest.raises(UnknownException) as exc_info:
|
||||
await repos_service.search_repositories(
|
||||
query='test',
|
||||
per_page=100,
|
||||
sort='stars',
|
||||
order='desc',
|
||||
public=False,
|
||||
app_mode=AppMode.SAAS,
|
||||
)
|
||||
|
||||
# Verify it's UnknownException, not RateLimitError
|
||||
assert isinstance(exc_info.value, UnknownException)
|
||||
assert not isinstance(exc_info.value, RateLimitError)
|
||||
Loading…
x
Reference in New Issue
Block a user