mirror of
https://github.com/OpenHands/OpenHands.git
synced 2025-12-26 05:48:36 +08:00
increase remote_runtime_api_timeout and handle duplicate secrets (#11239)
This commit is contained in:
parent
c62a6616db
commit
08118d742b
@ -349,18 +349,48 @@ class SaasNestedConversationManager(ConversationManager):
|
||||
api_url: str,
|
||||
custom_secrets: MappingProxyType[str, Any] | None,
|
||||
):
|
||||
"""Setup custom secrets for the nested conversation."""
|
||||
"""Setup custom secrets for the nested conversation.
|
||||
|
||||
Note: When resuming conversations, secrets may already exist in the runtime.
|
||||
We check for specific duplicate error messages to handle this case gracefully.
|
||||
"""
|
||||
if custom_secrets:
|
||||
for key, secret in custom_secrets.items():
|
||||
response = await client.post(
|
||||
f'{api_url}/api/secrets',
|
||||
json={
|
||||
'name': key,
|
||||
'description': secret.description,
|
||||
'value': secret.secret.get_secret_value(),
|
||||
},
|
||||
)
|
||||
response.raise_for_status()
|
||||
try:
|
||||
response = await client.post(
|
||||
f'{api_url}/api/secrets',
|
||||
json={
|
||||
'name': key,
|
||||
'description': secret.description,
|
||||
'value': secret.secret.get_secret_value(),
|
||||
},
|
||||
)
|
||||
response.raise_for_status()
|
||||
logger.debug(f'Successfully created secret: {key}')
|
||||
except httpx.HTTPStatusError as e:
|
||||
if e.response.status_code == 400:
|
||||
# Only ignore if it's actually a duplicate error
|
||||
try:
|
||||
error_data = e.response.json()
|
||||
error_msg = error_data.get('message', '')
|
||||
# The API returns: "Secret {secret_name} already exists"
|
||||
if 'already exists' in error_msg:
|
||||
logger.info(
|
||||
f'Secret "{key}" already exists, continuing - ignoring duplicate',
|
||||
extra={'api_url': api_url},
|
||||
)
|
||||
continue
|
||||
except (KeyError, ValueError, TypeError):
|
||||
pass # If we can't parse JSON, fall through to re-raise
|
||||
# Re-raise all other errors (including non-duplicate 400s)
|
||||
logger.error(
|
||||
f'Failed to setup secret "{key}": HTTP {e.response.status_code}',
|
||||
extra={
|
||||
'api_url': api_url,
|
||||
'response_text': e.response.text[:200],
|
||||
},
|
||||
)
|
||||
raise
|
||||
|
||||
def _get_mcp_config(self, user_id: str) -> MCPConfig | None:
|
||||
api_key_store = ApiKeyStore.get_instance()
|
||||
|
||||
176
enterprise/tests/unit/test_saas_conversation_manager_secrets.py
Normal file
176
enterprise/tests/unit/test_saas_conversation_manager_secrets.py
Normal file
@ -0,0 +1,176 @@
|
||||
"""Tests for SaasNestedConversationManager custom secrets handling during resume."""
|
||||
|
||||
from types import MappingProxyType
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
import httpx
|
||||
import pytest
|
||||
from pydantic import SecretStr
|
||||
from server.saas_nested_conversation_manager import SaasNestedConversationManager
|
||||
|
||||
from openhands.core.config.openhands_config import OpenHandsConfig
|
||||
from openhands.integrations.provider import CustomSecret
|
||||
from openhands.server.config.server_config import ServerConfig
|
||||
from openhands.storage.memory import InMemoryFileStore
|
||||
|
||||
|
||||
class MockHTTPXResponse:
|
||||
"""Mock httpx.Response that behaves realistically."""
|
||||
|
||||
def __init__(self, status_code: int, json_data: dict | None = None):
|
||||
self.status_code = status_code
|
||||
self._json_data = json_data or {}
|
||||
self.text = str(json_data) if json_data else ''
|
||||
|
||||
def json(self):
|
||||
"""Return JSON data."""
|
||||
if self._json_data:
|
||||
return self._json_data
|
||||
raise ValueError('No JSON data')
|
||||
|
||||
def raise_for_status(self):
|
||||
"""Raise an exception for 4xx/5xx status codes."""
|
||||
if self.status_code >= 400:
|
||||
# Create a proper mock response for the exception
|
||||
mock_response = MagicMock()
|
||||
mock_response.status_code = self.status_code
|
||||
mock_response.json = self.json
|
||||
mock_response.text = self.text
|
||||
|
||||
error = httpx.HTTPStatusError(
|
||||
f"Client error '{self.status_code}' for url 'test'",
|
||||
request=MagicMock(),
|
||||
response=mock_response,
|
||||
)
|
||||
raise error
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def saas_manager():
|
||||
"""Create a SaasNestedConversationManager instance for testing."""
|
||||
manager = SaasNestedConversationManager(
|
||||
sio=MagicMock(),
|
||||
config=MagicMock(spec=OpenHandsConfig),
|
||||
server_config=MagicMock(spec=ServerConfig),
|
||||
file_store=MagicMock(spec=InMemoryFileStore),
|
||||
event_retrieval=MagicMock(),
|
||||
)
|
||||
return manager
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_duplicate_secrets_dont_crash_resume(saas_manager):
|
||||
"""Test that duplicate secrets during resume are handled gracefully."""
|
||||
mock_client = AsyncMock(spec=httpx.AsyncClient)
|
||||
|
||||
# Simulate resume scenario: secret already exists (400)
|
||||
mock_response = MockHTTPXResponse(
|
||||
400, {'message': 'Secret MY_API_KEY already exists'}
|
||||
)
|
||||
|
||||
async def mock_post(*args, **kwargs):
|
||||
return mock_response
|
||||
|
||||
mock_client.post = AsyncMock(side_effect=mock_post)
|
||||
|
||||
custom_secrets = MappingProxyType(
|
||||
{
|
||||
'MY_API_KEY': CustomSecret(
|
||||
secret=SecretStr('api_key_value'),
|
||||
description='API Key that already exists on resume',
|
||||
),
|
||||
}
|
||||
)
|
||||
|
||||
# Should not raise despite 400 "already exists" error
|
||||
await saas_manager._setup_custom_secrets(
|
||||
client=mock_client,
|
||||
api_url='https://runtime.example.com',
|
||||
custom_secrets=custom_secrets,
|
||||
)
|
||||
|
||||
assert mock_client.post.call_count == 1
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_other_400_errors_still_fail(saas_manager):
|
||||
"""Test that non-duplicate 400 errors are still raised."""
|
||||
mock_client = AsyncMock(spec=httpx.AsyncClient)
|
||||
|
||||
# 400 error but NOT a duplicate
|
||||
mock_response = MockHTTPXResponse(400, {'message': 'Invalid secret name format'})
|
||||
|
||||
async def mock_post(*args, **kwargs):
|
||||
return mock_response
|
||||
|
||||
mock_client.post = AsyncMock(side_effect=mock_post)
|
||||
|
||||
custom_secrets = MappingProxyType(
|
||||
{
|
||||
'INVALID!NAME': CustomSecret(
|
||||
secret=SecretStr('value'), description='Secret with invalid name'
|
||||
),
|
||||
}
|
||||
)
|
||||
|
||||
with pytest.raises(httpx.HTTPStatusError) as exc_info:
|
||||
await saas_manager._setup_custom_secrets(
|
||||
client=mock_client,
|
||||
api_url='https://runtime.example.com',
|
||||
custom_secrets=custom_secrets,
|
||||
)
|
||||
|
||||
assert exc_info.value.response.status_code == 400
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_normal_secret_creation_still_works(saas_manager):
|
||||
"""Test that normal secret creation works correctly."""
|
||||
mock_client = AsyncMock(spec=httpx.AsyncClient)
|
||||
|
||||
# Successful creation
|
||||
mock_response = MockHTTPXResponse(200, {'message': 'Secret created'})
|
||||
|
||||
async def mock_post(*args, **kwargs):
|
||||
return mock_response
|
||||
|
||||
mock_client.post = AsyncMock(side_effect=mock_post)
|
||||
|
||||
custom_secrets = MappingProxyType(
|
||||
{
|
||||
'NEW_SECRET': CustomSecret(
|
||||
secret=SecretStr('new_value'), description='A new secret'
|
||||
),
|
||||
}
|
||||
)
|
||||
|
||||
await saas_manager._setup_custom_secrets(
|
||||
client=mock_client,
|
||||
api_url='https://runtime.example.com',
|
||||
custom_secrets=custom_secrets,
|
||||
)
|
||||
|
||||
assert mock_client.post.call_count == 1
|
||||
call_args = mock_client.post.call_args_list[0]
|
||||
assert call_args[1]['json']['name'] == 'NEW_SECRET'
|
||||
assert call_args[1]['json']['value'] == 'new_value'
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_handles_empty_secrets_gracefully(saas_manager):
|
||||
"""Test that empty or missing secrets are handled correctly."""
|
||||
mock_client = AsyncMock(spec=httpx.AsyncClient)
|
||||
|
||||
# Test with None
|
||||
await saas_manager._setup_custom_secrets(
|
||||
client=mock_client, api_url='https://runtime.example.com', custom_secrets=None
|
||||
)
|
||||
assert mock_client.post.call_count == 0
|
||||
|
||||
# Test with empty dict
|
||||
await saas_manager._setup_custom_secrets(
|
||||
client=mock_client,
|
||||
api_url='https://runtime.example.com',
|
||||
custom_secrets=MappingProxyType({}),
|
||||
)
|
||||
assert mock_client.post.call_count == 0
|
||||
@ -60,7 +60,7 @@ class SandboxConfig(BaseModel):
|
||||
logger.debug(f'SandboxConfig user_id default: {user_id}')
|
||||
timeout: int = Field(default=120)
|
||||
remote_runtime_init_timeout: int = Field(default=180)
|
||||
remote_runtime_api_timeout: int = Field(default=10)
|
||||
remote_runtime_api_timeout: int = Field(default=180)
|
||||
remote_runtime_enable_retries: bool = Field(default=True)
|
||||
remote_runtime_class: str | None = Field(
|
||||
default=None
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user