From 4c380e5a580f0a4771cc145278db6f39f44a504a Mon Sep 17 00:00:00 2001 From: Rohit Malhotra Date: Thu, 5 Mar 2026 19:02:04 -0500 Subject: [PATCH] feat: Add timeout handling for Slack repo query (#13249) Co-authored-by: openhands --- .../integrations/slack/slack_manager.py | 28 ++++- .../tests/unit/test_slack_integration.py | 119 +++++++++++++++++- .../integrations/protocols/http_client.py | 11 +- openhands/integrations/provider.py | 11 +- openhands/integrations/service_types.py | 6 + .../protocols/test_http_client.py | 19 ++- 6 files changed, 179 insertions(+), 15 deletions(-) diff --git a/enterprise/integrations/slack/slack_manager.py b/enterprise/integrations/slack/slack_manager.py index 17c57f7e6e..0db105805f 100644 --- a/enterprise/integrations/slack/slack_manager.py +++ b/enterprise/integrations/slack/slack_manager.py @@ -33,7 +33,7 @@ from storage.slack_user import SlackUser from openhands.core.logger import openhands_logger as logger from openhands.integrations.provider import ProviderHandler -from openhands.integrations.service_types import Repository +from openhands.integrations.service_types import ProviderTimeoutError, Repository from openhands.server.shared import config, server_config from openhands.server.types import ( LLMAuthenticationError, @@ -269,9 +269,31 @@ class SlackManager(Manager[SlackViewInterface]): return True elif isinstance(slack_view, SlackNewConversationView): user = slack_view.slack_to_openhands_user - user_repos: list[Repository] = await self._get_repositories( - slack_view.saas_user_auth + + # Fetch repositories, handling timeout errors from the provider + logger.info( + f'[Slack] Fetching repositories for user {user.slack_display_name} (id={slack_view.saas_user_auth.get_user_id()})' ) + try: + user_repos: list[Repository] = await self._get_repositories( + slack_view.saas_user_auth + ) + except ProviderTimeoutError: + logger.warning( + 'repo_query_timeout', + extra={ + 'slack_user_id': user.slack_user_id, + 'keycloak_user_id': user.keycloak_user_id, + }, + ) + timeout_msg = ( + 'The repository selection timed out while fetching your repository list. ' + 'Please re-send your message with a more specific repository name ' + '(e.g., "owner/repo-name") to help me find it faster.' + ) + await self.send_message(timeout_msg, slack_view, ephemeral=True) + return False + match, repos = self.filter_potential_repos_by_user_msg( slack_view.user_msg, user_repos ) diff --git a/enterprise/tests/unit/test_slack_integration.py b/enterprise/tests/unit/test_slack_integration.py index 255b730459..4797341a2e 100644 --- a/enterprise/tests/unit/test_slack_integration.py +++ b/enterprise/tests/unit/test_slack_integration.py @@ -1,7 +1,12 @@ -from unittest.mock import MagicMock +from unittest.mock import AsyncMock, MagicMock, patch import pytest from integrations.slack.slack_manager import SlackManager +from integrations.slack.slack_view import SlackNewConversationView +from storage.slack_user import SlackUser + +from openhands.integrations.service_types import ProviderTimeoutError +from openhands.server.user_auth.user_auth import UserAuth @pytest.fixture @@ -11,6 +16,46 @@ def slack_manager(): return slack_manager +@pytest.fixture +def mock_slack_user(): + """Create a mock SlackUser.""" + user = SlackUser() + user.slack_user_id = 'U1234567890' + user.keycloak_user_id = 'test-user-123' + user.slack_display_name = 'Test User' + return user + + +@pytest.fixture +def mock_user_auth(): + """Create a mock UserAuth.""" + auth = MagicMock(spec=UserAuth) + auth.get_provider_tokens = AsyncMock(return_value={}) + auth.get_secrets = AsyncMock(return_value=MagicMock(custom_secrets={})) + return auth + + +@pytest.fixture +def slack_new_conversation_view(mock_slack_user, mock_user_auth): + """Create a SlackNewConversationView instance for testing.""" + return SlackNewConversationView( + bot_access_token='xoxb-test-token', + user_msg='Hello OpenHands!', + slack_user_id='U1234567890', + slack_to_openhands_user=mock_slack_user, + saas_user_auth=mock_user_auth, + channel_id='C1234567890', + message_ts='1234567890.123456', + thread_ts=None, + selected_repo=None, + should_extract=True, + send_summary_instruction=True, + conversation_id='', + team_id='T1234567890', + v1_enabled=False, + ) + + @pytest.mark.parametrize( 'message,expected', [ @@ -23,3 +68,75 @@ def test_infer_repo_from_message(message, expected, slack_manager): # Test the extracted function result = slack_manager._infer_repo_from_message(message) assert result == expected + + +class TestRepoQueryTimeoutHandling: + """Test timeout handling when fetching repositories for Slack integration.""" + + @patch.object(SlackManager, 'send_message', new_callable=AsyncMock) + @patch.object(SlackManager, '_get_repositories', new_callable=AsyncMock) + async def test_timeout_sends_user_friendly_message( + self, + mock_get_repositories, + mock_send_message, + slack_manager, + slack_new_conversation_view, + ): + """Test that when repository fetching times out, a user-friendly message is sent.""" + # Setup: _get_repositories raises ProviderTimeoutError + mock_get_repositories.side_effect = ProviderTimeoutError( + 'github API request timed out: ConnectTimeout' + ) + + # Execute + result = await slack_manager.is_job_requested( + MagicMock(), slack_new_conversation_view + ) + + # Verify: should return False (job not started) + assert result is False + + # Verify: send_message was called with the timeout message + mock_send_message.assert_called_once() + call_args = mock_send_message.call_args + + # Check the message content + message = call_args[0][0] + assert 'timed out' in message + assert 'repository name' in message + assert 'owner/repo-name' in message + + # Check it was sent as ephemeral + assert call_args[1]['ephemeral'] is True + + @patch.object(SlackManager, 'send_message', new_callable=AsyncMock) + @patch.object(SlackManager, '_get_repositories', new_callable=AsyncMock) + async def test_successful_repo_fetch_does_not_send_timeout_message( + self, + mock_get_repositories, + mock_send_message, + slack_manager, + slack_new_conversation_view, + ): + """Test that successful repo fetch shows repo selector, not timeout message.""" + # Setup: _get_repositories returns empty list (no repos, but no timeout) + mock_get_repositories.return_value = [] + + # Execute + result = await slack_manager.is_job_requested( + MagicMock(), slack_new_conversation_view + ) + + # Verify: should return False (no repo selected yet) + assert result is False + + # Verify: send_message was called (for repo selector) + mock_send_message.assert_called_once() + call_args = mock_send_message.call_args + + # Check the message is NOT the timeout message + message = call_args[0][0] + assert 'timed out' not in str(message) + # Should be the repo selection form + assert isinstance(message, dict) + assert message.get('text') == 'Choose a Repository:' diff --git a/openhands/integrations/protocols/http_client.py b/openhands/integrations/protocols/http_client.py index 5b12da029e..92dba3dade 100644 --- a/openhands/integrations/protocols/http_client.py +++ b/openhands/integrations/protocols/http_client.py @@ -3,12 +3,13 @@ from abc import ABC, abstractmethod from typing import Any -from httpx import AsyncClient, HTTPError, HTTPStatusError +from httpx import AsyncClient, HTTPError, HTTPStatusError, TimeoutException from pydantic import SecretStr from openhands.core.logger import openhands_logger as logger from openhands.integrations.service_types import ( AuthenticationError, + ProviderTimeoutError, RateLimitError, RequestMethod, ResourceNotFoundError, @@ -93,7 +94,13 @@ class HTTPClient(ABC): logger.warning(f'Status error on {self.provider} API: {e}') return UnknownException(f'Unknown error: {e}') - def handle_http_error(self, e: HTTPError) -> UnknownException: + def handle_http_error( + self, e: HTTPError + ) -> ProviderTimeoutError | UnknownException: """Handle general HTTP errors.""" logger.warning(f'HTTP error on {self.provider} API: {type(e).__name__} : {e}') + if isinstance(e, TimeoutException): + return ProviderTimeoutError( + f'{self.provider} API request timed out: {type(e).__name__}' + ) return UnknownException(f'HTTP error {type(e).__name__} : {e}') diff --git a/openhands/integrations/provider.py b/openhands/integrations/provider.py index ad94305b3c..aa210eae18 100644 --- a/openhands/integrations/provider.py +++ b/openhands/integrations/provider.py @@ -35,6 +35,7 @@ from openhands.integrations.service_types import ( InstallationsService, MicroagentParseError, PaginatedBranchesResponse, + ProviderTimeoutError, ProviderType, Repository, ResourceNotFoundError, @@ -258,9 +259,10 @@ class ProviderHandler: per_page: int | None, installation_id: str | None, ) -> list[Repository]: - """Get repositories from providers""" - """ - Get repositories from providers + """Get repositories from providers. + + Raises: + ProviderTimeoutError: If a timeout occurs while fetching repos. """ if selected_provider: if not page or not per_page: @@ -277,6 +279,9 @@ class ProviderHandler: service = self.get_service(provider) service_repos = await service.get_all_repositories(sort, app_mode) all_repos.extend(service_repos) + except ProviderTimeoutError: + # Propagate timeout errors so callers can handle them appropriately + raise except Exception as e: logger.warning(f'Error fetching repos from {provider}: {e}') diff --git a/openhands/integrations/service_types.py b/openhands/integrations/service_types.py index 27ae0e5edb..38f7a83da5 100644 --- a/openhands/integrations/service_types.py +++ b/openhands/integrations/service_types.py @@ -191,6 +191,12 @@ class RateLimitError(ValueError): pass +class ProviderTimeoutError(ValueError): + """Raised when a request to a git provider times out.""" + + pass + + class ResourceNotFoundError(ValueError): """Raised when a requested resource (file, directory, etc.) is not found.""" diff --git a/tests/unit/integrations/protocols/test_http_client.py b/tests/unit/integrations/protocols/test_http_client.py index 1210d34344..c6ce529c9d 100644 --- a/tests/unit/integrations/protocols/test_http_client.py +++ b/tests/unit/integrations/protocols/test_http_client.py @@ -10,6 +10,7 @@ from pydantic import SecretStr from openhands.integrations.protocols.http_client import HTTPClient from openhands.integrations.service_types import ( AuthenticationError, + ProviderTimeoutError, RateLimitError, RequestMethod, ResourceNotFoundError, @@ -201,18 +202,24 @@ class TestHTTPClient: client = TestableHTTPClient() client.provider = 'test-provider' - # Test with different error types - errors = [ - httpx.ConnectError('Connection failed'), + # Test with non-timeout error (should return UnknownException) + connect_error = httpx.ConnectError('Connection failed') + result = client.handle_http_error(connect_error) + assert isinstance(result, UnknownException) + assert 'HTTP error ConnectError' in str(result) + + # Test with timeout errors (should return ProviderTimeoutError) + timeout_errors = [ httpx.TimeoutException('Request timed out'), httpx.ReadTimeout('Read timeout'), httpx.WriteTimeout('Write timeout'), ] - for error in errors: + for error in timeout_errors: result = client.handle_http_error(error) - assert isinstance(result, UnknownException) - assert f'HTTP error {type(error).__name__}' in str(result) + assert isinstance(result, ProviderTimeoutError) + assert 'test-provider API request timed out' in str(result) + assert type(error).__name__ in str(result) def test_runtime_checkable(self): """Test that HTTPClient is runtime checkable."""