diff --git a/openhands/app_server/sandbox/remote_sandbox_service.py b/openhands/app_server/sandbox/remote_sandbox_service.py index c7d444c4ec..c96b7362c0 100644 --- a/openhands/app_server/sandbox/remote_sandbox_service.py +++ b/openhands/app_server/sandbox/remote_sandbox_service.py @@ -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', diff --git a/openhands/server/routes/manage_conversations.py b/openhands/server/routes/manage_conversations.py index 0b828c76c0..7d0b1f1c0c 100644 --- a/openhands/server/routes/manage_conversations.py +++ b/openhands/server/routes/manage_conversations.py @@ -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) diff --git a/tests/unit/app_server/test_remote_sandbox_service.py b/tests/unit/app_server/test_remote_sandbox_service.py index 1d917cc760..567ecad2e3 100644 --- a/tests/unit/app_server/test_remote_sandbox_service.py +++ b/tests/unit/app_server/test_remote_sandbox_service.py @@ -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', diff --git a/tests/unit/server/data_models/test_conversation.py b/tests/unit/server/data_models/test_conversation.py index 0917dc1fac..c3cf34dac3 100644 --- a/tests/unit/server/data_models/test_conversation.py +++ b/tests/unit/server/data_models/test_conversation.py @@ -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