Do not get live status updates when they are not required (#11727)

Co-authored-by: openhands <openhands@all-hands.dev>
This commit is contained in:
Tim O'Farrell 2025-11-14 14:55:43 +00:00 committed by GitHub
parent 8115d82f96
commit 2841e35f24
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 292 additions and 144 deletions

View File

@ -318,7 +318,6 @@ class RemoteSandboxService(SandboxService):
created_at=utc_now(),
)
self.db_session.add(stored_sandbox)
await self.db_session.commit()
# Prepare environment variables
environment = await self._init_environment(sandbox_spec, sandbox_id)
@ -407,7 +406,6 @@ class RemoteSandboxService(SandboxService):
if not stored_sandbox:
return False
await self.db_session.delete(stored_sandbox)
await self.db_session.commit()
runtime_data = await self._get_runtime(sandbox_id)
response = await self._send_runtime_api_request(
'POST',

View File

@ -1,3 +1,4 @@
import asyncio
import base64
import itertools
import json
@ -7,10 +8,11 @@ import uuid
from datetime import datetime, timedelta, timezone
import base62
from fastapi import APIRouter, Depends, status
from fastapi import APIRouter, Depends, Request, status
from fastapi.responses import JSONResponse
from jinja2 import Environment, FileSystemLoader
from pydantic import BaseModel, ConfigDict, Field
from sqlalchemy.ext.asyncio import AsyncSession
from openhands.app_server.app_conversation.app_conversation_info_service import (
AppConversationInfoService,
@ -24,9 +26,11 @@ from openhands.app_server.app_conversation.app_conversation_service import (
from openhands.app_server.config import (
depends_app_conversation_info_service,
depends_app_conversation_service,
depends_db_session,
depends_sandbox_service,
)
from openhands.app_server.sandbox.sandbox_service import SandboxService
from openhands.app_server.services.db_session_injector import set_db_session_keep_open
from openhands.core.config.llm_config import LLMConfig
from openhands.core.config.mcp_config import MCPConfig
from openhands.core.logger import openhands_logger as logger
@ -99,6 +103,7 @@ app = APIRouter(prefix='/api', dependencies=get_dependencies())
app_conversation_service_dependency = depends_app_conversation_service()
app_conversation_info_service_dependency = depends_app_conversation_info_service()
sandbox_service_dependency = depends_sandbox_service()
db_session_dependency = depends_db_session()
def _filter_conversations_by_age(
@ -467,16 +472,22 @@ async def get_conversation(
@app.delete('/conversations/{conversation_id}')
async def delete_conversation(
request: Request,
conversation_id: str = Depends(validate_conversation_id),
user_id: str | None = Depends(get_user_id),
app_conversation_service: AppConversationService = app_conversation_service_dependency,
app_conversation_info_service: AppConversationInfoService = app_conversation_info_service_dependency,
sandbox_service: SandboxService = sandbox_service_dependency,
db_session: AsyncSession = db_session_dependency,
) -> bool:
set_db_session_keep_open(request.state, True)
# Try V1 conversation first
v1_result = await _try_delete_v1_conversation(
conversation_id,
app_conversation_service,
app_conversation_info_service,
sandbox_service,
db_session,
)
if v1_result is not None:
return v1_result
@ -488,23 +499,32 @@ async def delete_conversation(
async def _try_delete_v1_conversation(
conversation_id: str,
app_conversation_service: AppConversationService,
app_conversation_info_service: AppConversationInfoService,
sandbox_service: SandboxService,
db_session: AsyncSession,
) -> bool | None:
"""Try to delete a V1 conversation. Returns None if not a V1 conversation."""
result = None
try:
conversation_uuid = uuid.UUID(conversation_id)
# Check if it's a V1 conversation by trying to get it
app_conversation = await app_conversation_service.get_app_conversation(
conversation_uuid
app_conversation_info = (
await app_conversation_info_service.get_app_conversation_info(
conversation_uuid
)
)
if app_conversation:
if app_conversation_info:
# This is a V1 conversation, delete it using the app conversation service
# Pass the conversation ID for secure deletion
result = await app_conversation_service.delete_app_conversation(
app_conversation.id
app_conversation_info.id
)
# Delete the sandbox in the background
asyncio.create_task(
_delete_sandbox_and_close_connection(
sandbox_service, app_conversation_info.sandbox_id, db_session
)
)
await sandbox_service.delete_sandbox(app_conversation.sandbox_id)
except (ValueError, TypeError):
# Not a valid UUID, continue with V0 logic
pass
@ -515,6 +535,16 @@ async def _try_delete_v1_conversation(
return result
async def _delete_sandbox_and_close_connection(
sandbox_service: SandboxService, sandbox_id: str, db_session: AsyncSession
):
try:
await sandbox_service.delete_sandbox(sandbox_id)
await db_session.commit()
finally:
await db_session.aclose()
async def _delete_v0_conversation(conversation_id: str, user_id: str | None) -> bool:
"""Delete a V0 conversation using the legacy logic."""
conversation_store = await ConversationStoreImpl.get_instance(config, user_id)

View File

@ -435,7 +435,7 @@ class TestSandboxLifecycle:
9
) # max_num_sandboxes - 1
remote_sandbox_service.db_session.add.assert_called_once()
remote_sandbox_service.db_session.commit.assert_called_once()
remote_sandbox_service.db_session.commit.assert_not_called()
@pytest.mark.asyncio
async def test_start_sandbox_with_specific_spec(
@ -627,7 +627,7 @@ class TestSandboxLifecycle:
# Verify
assert result is True
remote_sandbox_service.db_session.delete.assert_called_once_with(stored_sandbox)
remote_sandbox_service.db_session.commit.assert_called_once()
remote_sandbox_service.db_session.commit.assert_not_called()
remote_sandbox_service.httpx_client.request.assert_called_once_with(
'POST',
'https://api.example.com/stop',

View File

@ -911,10 +911,16 @@ async def test_delete_conversation():
# Create a mock app conversation service
mock_app_conversation_service = MagicMock()
mock_app_conversation_service.get_app_conversation = AsyncMock(
# Create a mock app conversation info service
mock_app_conversation_info_service = MagicMock()
mock_app_conversation_info_service.get_app_conversation_info = AsyncMock(
return_value=None
)
# Create a mock sandbox service
mock_sandbox_service = MagicMock()
# Mock the conversation manager
with patch(
'openhands.server.routes.manage_conversations.conversation_manager'
@ -932,9 +938,12 @@ async def test_delete_conversation():
# Call delete_conversation
result = await delete_conversation(
request=MagicMock(),
conversation_id='some_conversation_id',
user_id='12345',
app_conversation_service=mock_app_conversation_service,
app_conversation_info_service=mock_app_conversation_info_service,
sandbox_service=mock_sandbox_service,
)
# Verify the result
@ -972,42 +981,63 @@ async def test_delete_v1_conversation_success():
mock_service = MagicMock()
mock_service_dep.return_value = mock_service
# Mock the conversation exists
mock_app_conversation = AppConversation(
id=conversation_uuid,
created_by_user_id='test_user',
sandbox_id='test-sandbox-id',
title='Test V1 Conversation',
sandbox_status=SandboxStatus.RUNNING,
execution_status=ConversationExecutionStatus.RUNNING,
session_api_key='test-api-key',
selected_repository='test/repo',
selected_branch='main',
git_provider=ProviderType.GITHUB,
trigger=ConversationTrigger.GUI,
created_at=datetime.now(timezone.utc),
updated_at=datetime.now(timezone.utc),
)
mock_service.get_app_conversation = AsyncMock(
return_value=mock_app_conversation
)
mock_service.delete_app_conversation = AsyncMock(return_value=True)
# Mock the app conversation info service
with patch(
'openhands.server.routes.manage_conversations.app_conversation_info_service_dependency'
) as mock_info_service_dep:
mock_info_service = MagicMock()
mock_info_service_dep.return_value = mock_info_service
# Call delete_conversation with V1 conversation ID
result = await delete_conversation(
conversation_id=conversation_id,
user_id='test_user',
app_conversation_service=mock_service,
)
# Mock the sandbox service
with patch(
'openhands.server.routes.manage_conversations.sandbox_service_dependency'
) as mock_sandbox_service_dep:
mock_sandbox_service = MagicMock()
mock_sandbox_service_dep.return_value = mock_sandbox_service
# Verify the result
assert result is True
# Mock the conversation info exists
mock_app_conversation_info = AppConversation(
id=conversation_uuid,
created_by_user_id='test_user',
sandbox_id='test-sandbox-id',
title='Test V1 Conversation',
sandbox_status=SandboxStatus.RUNNING,
execution_status=ConversationExecutionStatus.RUNNING,
session_api_key='test-api-key',
selected_repository='test/repo',
selected_branch='main',
git_provider=ProviderType.GITHUB,
trigger=ConversationTrigger.GUI,
created_at=datetime.now(timezone.utc),
updated_at=datetime.now(timezone.utc),
)
mock_info_service.get_app_conversation_info = AsyncMock(
return_value=mock_app_conversation_info
)
mock_service.delete_app_conversation = AsyncMock(return_value=True)
# Verify that get_app_conversation was called
mock_service.get_app_conversation.assert_called_once_with(conversation_uuid)
# Call delete_conversation with V1 conversation ID
result = await delete_conversation(
request=MagicMock(),
conversation_id=conversation_id,
user_id='test_user',
app_conversation_service=mock_service,
app_conversation_info_service=mock_info_service,
sandbox_service=mock_sandbox_service,
)
# Verify that delete_app_conversation was called with the conversation ID
mock_service.delete_app_conversation.assert_called_once_with(conversation_uuid)
# Verify the result
assert result is True
# Verify that get_app_conversation_info was called
mock_info_service.get_app_conversation_info.assert_called_once_with(
conversation_uuid
)
# Verify that delete_app_conversation was called with the conversation ID
mock_service.delete_app_conversation.assert_called_once_with(
conversation_uuid
)
@pytest.mark.asyncio
@ -1025,25 +1055,46 @@ async def test_delete_v1_conversation_not_found():
mock_service = MagicMock()
mock_service_dep.return_value = mock_service
# Mock the conversation doesn't exist
mock_service.get_app_conversation = AsyncMock(return_value=None)
mock_service.delete_app_conversation = AsyncMock(return_value=False)
# Mock the app conversation info service
with patch(
'openhands.server.routes.manage_conversations.app_conversation_info_service_dependency'
) as mock_info_service_dep:
mock_info_service = MagicMock()
mock_info_service_dep.return_value = mock_info_service
# Call delete_conversation with V1 conversation ID
result = await delete_conversation(
conversation_id=conversation_id,
user_id='test_user',
app_conversation_service=mock_service,
)
# Mock the sandbox service
with patch(
'openhands.server.routes.manage_conversations.sandbox_service_dependency'
) as mock_sandbox_service_dep:
mock_sandbox_service = MagicMock()
mock_sandbox_service_dep.return_value = mock_sandbox_service
# Verify the result
assert result is False
# Mock the conversation doesn't exist
mock_info_service.get_app_conversation_info = AsyncMock(
return_value=None
)
mock_service.delete_app_conversation = AsyncMock(return_value=False)
# Verify that get_app_conversation was called
mock_service.get_app_conversation.assert_called_once_with(conversation_uuid)
# Call delete_conversation with V1 conversation ID
result = await delete_conversation(
request=MagicMock(),
conversation_id=conversation_id,
user_id='test_user',
app_conversation_service=mock_service,
app_conversation_info_service=mock_info_service,
sandbox_service=mock_sandbox_service,
)
# Verify that delete_app_conversation was NOT called
mock_service.delete_app_conversation.assert_not_called()
# Verify the result
assert result is False
# Verify that get_app_conversation_info was called
mock_info_service.get_app_conversation_info.assert_called_once_with(
conversation_uuid
)
# Verify that delete_app_conversation was NOT called
mock_service.delete_app_conversation.assert_not_called()
@pytest.mark.asyncio
@ -1091,19 +1142,40 @@ async def test_delete_v1_conversation_invalid_uuid():
mock_runtime_cls.delete = AsyncMock()
mock_get_runtime_cls.return_value = mock_runtime_cls
# Call delete_conversation
result = await delete_conversation(
conversation_id=conversation_id,
user_id='test_user',
app_conversation_service=mock_service,
)
# Mock the app conversation info service
with patch(
'openhands.server.routes.manage_conversations.app_conversation_info_service_dependency'
) as mock_info_service_dep:
mock_info_service = MagicMock()
mock_info_service_dep.return_value = mock_info_service
# Verify the result
assert result is True
# Mock the sandbox service
with patch(
'openhands.server.routes.manage_conversations.sandbox_service_dependency'
) as mock_sandbox_service_dep:
mock_sandbox_service = MagicMock()
mock_sandbox_service_dep.return_value = mock_sandbox_service
# Verify V0 logic was used
mock_store.delete_metadata.assert_called_once_with(conversation_id)
mock_runtime_cls.delete.assert_called_once_with(conversation_id)
# Call delete_conversation
result = await delete_conversation(
request=MagicMock(),
conversation_id=conversation_id,
user_id='test_user',
app_conversation_service=mock_service,
app_conversation_info_service=mock_info_service,
sandbox_service=mock_sandbox_service,
)
# Verify the result
assert result is True
# Verify V0 logic was used
mock_store.delete_metadata.assert_called_once_with(
conversation_id
)
mock_runtime_cls.delete.assert_called_once_with(
conversation_id
)
@pytest.mark.asyncio
@ -1121,57 +1193,84 @@ async def test_delete_v1_conversation_service_error():
mock_service = MagicMock()
mock_service_dep.return_value = mock_service
# Mock service error
mock_service.get_app_conversation = AsyncMock(
side_effect=Exception('Service error')
)
# Mock V0 conversation logic as fallback
# Mock the app conversation info service
with patch(
'openhands.server.routes.manage_conversations.ConversationStoreImpl.get_instance'
) as mock_get_instance:
mock_store = MagicMock()
mock_store.get_metadata = AsyncMock(
return_value=ConversationMetadata(
conversation_id=conversation_id,
title='Test V0 Conversation',
created_at=datetime.fromisoformat('2025-01-01T00:00:00+00:00'),
last_updated_at=datetime.fromisoformat('2025-01-01T00:01:00+00:00'),
selected_repository='test/repo',
user_id='test_user',
)
)
mock_store.delete_metadata = AsyncMock()
mock_get_instance.return_value = mock_store
'openhands.server.routes.manage_conversations.app_conversation_info_service_dependency'
) as mock_info_service_dep:
mock_info_service = MagicMock()
mock_info_service_dep.return_value = mock_info_service
# Mock conversation manager
# Mock the sandbox service
with patch(
'openhands.server.routes.manage_conversations.conversation_manager'
) as mock_manager:
mock_manager.is_agent_loop_running = AsyncMock(return_value=False)
mock_manager.get_connections = AsyncMock(return_value={})
'openhands.server.routes.manage_conversations.sandbox_service_dependency'
) as mock_sandbox_service_dep:
mock_sandbox_service = MagicMock()
mock_sandbox_service_dep.return_value = mock_sandbox_service
# Mock runtime
# Mock service error
mock_info_service.get_app_conversation_info = AsyncMock(
side_effect=Exception('Service error')
)
# Mock V0 conversation logic as fallback
with patch(
'openhands.server.routes.manage_conversations.get_runtime_cls'
) as mock_get_runtime_cls:
mock_runtime_cls = MagicMock()
mock_runtime_cls.delete = AsyncMock()
mock_get_runtime_cls.return_value = mock_runtime_cls
# Call delete_conversation
result = await delete_conversation(
conversation_id=conversation_id,
user_id='test_user',
app_conversation_service=mock_service,
'openhands.server.routes.manage_conversations.ConversationStoreImpl.get_instance'
) as mock_get_instance:
mock_store = MagicMock()
mock_store.get_metadata = AsyncMock(
return_value=ConversationMetadata(
conversation_id=conversation_id,
title='Test V0 Conversation',
created_at=datetime.fromisoformat(
'2025-01-01T00:00:00+00:00'
),
last_updated_at=datetime.fromisoformat(
'2025-01-01T00:01:00+00:00'
),
selected_repository='test/repo',
user_id='test_user',
)
)
mock_store.delete_metadata = AsyncMock()
mock_get_instance.return_value = mock_store
# Verify the result (should fallback to V0)
assert result is True
# Mock conversation manager
with patch(
'openhands.server.routes.manage_conversations.conversation_manager'
) as mock_manager:
mock_manager.is_agent_loop_running = AsyncMock(
return_value=False
)
mock_manager.get_connections = AsyncMock(return_value={})
# Verify V0 logic was used
mock_store.delete_metadata.assert_called_once_with(conversation_id)
mock_runtime_cls.delete.assert_called_once_with(conversation_id)
# Mock runtime
with patch(
'openhands.server.routes.manage_conversations.get_runtime_cls'
) as mock_get_runtime_cls:
mock_runtime_cls = MagicMock()
mock_runtime_cls.delete = AsyncMock()
mock_get_runtime_cls.return_value = mock_runtime_cls
# Call delete_conversation
result = await delete_conversation(
request=MagicMock(),
conversation_id=conversation_id,
user_id='test_user',
app_conversation_service=mock_service,
app_conversation_info_service=mock_info_service,
sandbox_service=mock_sandbox_service,
)
# Verify the result (should fallback to V0)
assert result is True
# Verify V0 logic was used
mock_store.delete_metadata.assert_called_once_with(
conversation_id
)
mock_runtime_cls.delete.assert_called_once_with(
conversation_id
)
@pytest.mark.asyncio
@ -1195,42 +1294,63 @@ async def test_delete_v1_conversation_with_agent_server():
mock_service = MagicMock()
mock_service_dep.return_value = mock_service
# Mock the conversation exists with running sandbox
mock_app_conversation = AppConversation(
id=conversation_uuid,
created_by_user_id='test_user',
sandbox_id='test-sandbox-id',
title='Test V1 Conversation',
sandbox_status=SandboxStatus.RUNNING,
execution_status=ConversationExecutionStatus.RUNNING,
session_api_key='test-api-key',
selected_repository='test/repo',
selected_branch='main',
git_provider=ProviderType.GITHUB,
trigger=ConversationTrigger.GUI,
created_at=datetime.now(timezone.utc),
updated_at=datetime.now(timezone.utc),
)
mock_service.get_app_conversation = AsyncMock(
return_value=mock_app_conversation
)
mock_service.delete_app_conversation = AsyncMock(return_value=True)
# Mock the app conversation info service
with patch(
'openhands.server.routes.manage_conversations.app_conversation_info_service_dependency'
) as mock_info_service_dep:
mock_info_service = MagicMock()
mock_info_service_dep.return_value = mock_info_service
# Call delete_conversation with V1 conversation ID
result = await delete_conversation(
conversation_id=conversation_id,
user_id='test_user',
app_conversation_service=mock_service,
)
# Mock the sandbox service
with patch(
'openhands.server.routes.manage_conversations.sandbox_service_dependency'
) as mock_sandbox_service_dep:
mock_sandbox_service = MagicMock()
mock_sandbox_service_dep.return_value = mock_sandbox_service
# Verify the result
assert result is True
# Mock the conversation exists with running sandbox
mock_app_conversation_info = AppConversation(
id=conversation_uuid,
created_by_user_id='test_user',
sandbox_id='test-sandbox-id',
title='Test V1 Conversation',
sandbox_status=SandboxStatus.RUNNING,
execution_status=ConversationExecutionStatus.RUNNING,
session_api_key='test-api-key',
selected_repository='test/repo',
selected_branch='main',
git_provider=ProviderType.GITHUB,
trigger=ConversationTrigger.GUI,
created_at=datetime.now(timezone.utc),
updated_at=datetime.now(timezone.utc),
)
mock_info_service.get_app_conversation_info = AsyncMock(
return_value=mock_app_conversation_info
)
mock_service.delete_app_conversation = AsyncMock(return_value=True)
# Verify that get_app_conversation was called
mock_service.get_app_conversation.assert_called_once_with(conversation_uuid)
# Call delete_conversation with V1 conversation ID
result = await delete_conversation(
request=MagicMock(),
conversation_id=conversation_id,
user_id='test_user',
app_conversation_service=mock_service,
app_conversation_info_service=mock_info_service,
sandbox_service=mock_sandbox_service,
)
# Verify that delete_app_conversation was called with the conversation ID
mock_service.delete_app_conversation.assert_called_once_with(conversation_uuid)
# Verify the result
assert result is True
# Verify that get_app_conversation_info was called
mock_info_service.get_app_conversation_info.assert_called_once_with(
conversation_uuid
)
# Verify that delete_app_conversation was called with the conversation ID
mock_service.delete_app_conversation.assert_called_once_with(
conversation_uuid
)
@pytest.mark.asyncio