From bb064e9907d5a0e191cb1a9ba95a0988d54ad8b1 Mon Sep 17 00:00:00 2001 From: openhands Date: Fri, 14 Nov 2025 12:42:33 +0000 Subject: [PATCH] Fix unit tests for background delete conversation process - Update delete_conversation function to use BackgroundTasks parameter - Add sandbox_service parameter for V1 conversation handling - Fix all 6 conversation deletion tests to work with background task pattern - Update test mocks to use AsyncMock for proper async function testing - Add proper exception handling for V1 deletion with V0 fallback Fixes failing tests: - test_delete_conversation - test_delete_v1_conversation_success - test_delete_v1_conversation_not_found - test_delete_v1_conversation_invalid_uuid - test_delete_v1_conversation_service_error - test_delete_v1_conversation_with_agent_server Co-authored-by: openhands --- .../server/routes/manage_conversations.py | 116 ++++++--- .../server/data_models/test_conversation.py | 226 +++++++++++++----- 2 files changed, 257 insertions(+), 85 deletions(-) diff --git a/openhands/server/routes/manage_conversations.py b/openhands/server/routes/manage_conversations.py index 0b828c76c0..77c319f820 100644 --- a/openhands/server/routes/manage_conversations.py +++ b/openhands/server/routes/manage_conversations.py @@ -7,7 +7,7 @@ import uuid from datetime import datetime, timedelta, timezone import base62 -from fastapi import APIRouter, Depends, status +from fastapi import APIRouter, BackgroundTasks, Depends, status from fastapi.responses import JSONResponse from jinja2 import Environment, FileSystemLoader from pydantic import BaseModel, ConfigDict, Field @@ -467,13 +467,15 @@ async def get_conversation( @app.delete('/conversations/{conversation_id}') async def delete_conversation( + background_tasks: BackgroundTasks, conversation_id: str = Depends(validate_conversation_id), user_id: str | None = Depends(get_user_id), app_conversation_service: AppConversationService = app_conversation_service_dependency, sandbox_service: SandboxService = sandbox_service_dependency, ) -> bool: - # Try V1 conversation first - v1_result = await _try_delete_v1_conversation( + # Try V1 conversation first - do validation synchronously + v1_result = await _check_and_schedule_v1_deletion( + background_tasks, conversation_id, app_conversation_service, sandbox_service, @@ -481,38 +483,98 @@ async def delete_conversation( if v1_result is not None: return v1_result - # V0 conversation logic - return await _delete_v0_conversation(conversation_id, user_id) + # V0 conversation logic - do validation synchronously, schedule cleanup in background + return await _check_and_schedule_v0_deletion(background_tasks, conversation_id, user_id) -async def _try_delete_v1_conversation( +async def _check_and_schedule_v1_deletion( + background_tasks: BackgroundTasks, conversation_id: str, app_conversation_service: AppConversationService, sandbox_service: SandboxService, ) -> bool | None: - """Try to delete a V1 conversation. Returns None if not a V1 conversation.""" - result = None + """Check if V1 conversation exists and schedule deletion if found.""" 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 - ) - if app_conversation: - # 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 - ) - await sandbox_service.delete_sandbox(app_conversation.sandbox_id) - except (ValueError, TypeError): - # Not a valid UUID, continue with V0 logic - pass - except Exception: - # Some other error, continue with V0 logic - pass + conversation_uuid = UUID(conversation_id) + except ValueError: + return None # Not a valid UUID, fall back to V0 logic + + try: + # Check if conversation exists + app_conversation = await app_conversation_service.get_app_conversation(conversation_uuid) + if app_conversation is None: + return False # V1 conversation not found + + # Schedule the actual deletion work in background + background_tasks.add_task( + _perform_v1_deletion, + conversation_uuid, + app_conversation_service, + sandbox_service, + app_conversation.sandbox_id, + ) + return True + except Exception: + # Service error, fall back to V0 logic + return None + + +async def _check_and_schedule_v0_deletion( + background_tasks: BackgroundTasks, + conversation_id: str, + user_id: str | None, +) -> bool: + """Check if V0 conversation exists and schedule deletion if found.""" + # Check if conversation exists in V0 store + conversation_store = ConversationStoreImpl.get_instance() + try: + metadata = await conversation_store.get_metadata(conversation_id) + if metadata is None: + return False # V0 conversation not found + except Exception: + return False # Error getting metadata, assume not found + + # Schedule the actual deletion work in background + background_tasks.add_task( + _perform_v0_deletion, + conversation_id, + user_id, + ) + return True + + +async def _perform_v1_deletion( + conversation_uuid: UUID, + app_conversation_service: AppConversationService, + sandbox_service: SandboxService, + sandbox_id: str, +) -> None: + """Perform V1 conversation deletion work in the background.""" + try: + # Delete the app conversation + await app_conversation_service.delete_app_conversation(conversation_uuid) + + # Delete the sandbox + if sandbox_id: + await sandbox_service.delete_sandbox(sandbox_id) + except Exception as e: + # Log the error but don't raise it since this is running in background + print(f"Error deleting V1 conversation {conversation_uuid}: {e}") + + +async def _perform_v0_deletion( + conversation_id: str, + user_id: str | None, +) -> None: + """Perform V0 conversation deletion work in the background.""" + try: + await _delete_v0_conversation(conversation_id, user_id) + except Exception as e: + # Log the error but don't raise it since this is running in background + print(f"Error deleting V0 conversation {conversation_id}: {e}") + + - return result async def _delete_v0_conversation(conversation_id: str, user_id: str | None) -> bool: diff --git a/tests/unit/server/data_models/test_conversation.py b/tests/unit/server/data_models/test_conversation.py index 0917dc1fac..89c6f12ad9 100644 --- a/tests/unit/server/data_models/test_conversation.py +++ b/tests/unit/server/data_models/test_conversation.py @@ -930,16 +930,30 @@ async def test_delete_conversation(): mock_runtime_cls.delete = AsyncMock() mock_get_runtime_cls.return_value = mock_runtime_cls + # Create a mock BackgroundTasks + from fastapi import BackgroundTasks + background_tasks = BackgroundTasks() + + # Create a mock sandbox service + mock_sandbox_service = MagicMock() + # Call delete_conversation result = await delete_conversation( + background_tasks=background_tasks, conversation_id='some_conversation_id', user_id='12345', app_conversation_service=mock_app_conversation_service, + sandbox_service=mock_sandbox_service, ) # Verify the result assert result is True + # Execute the background task that was scheduled + assert len(background_tasks.tasks) == 1 + task_func, task_args, task_kwargs = background_tasks.tasks[0] + await task_func(*task_args, **task_kwargs) + # Verify that delete_metadata was called mock_store.delete_metadata.assert_called_once_with( 'some_conversation_id' @@ -993,21 +1007,43 @@ async def test_delete_v1_conversation_success(): ) mock_service.delete_app_conversation = AsyncMock(return_value=True) - # 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 sandbox service + with patch( + 'openhands.server.routes.manage_conversations.sandbox_service_dependency' + ) as mock_sandbox_dep: + mock_sandbox_service = MagicMock() + mock_sandbox_service.delete_sandbox = AsyncMock() + mock_sandbox_dep.return_value = mock_sandbox_service - # Verify the result - assert result is True + # Create a mock BackgroundTasks + from fastapi import BackgroundTasks + background_tasks = BackgroundTasks() - # 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( + background_tasks=background_tasks, + conversation_id=conversation_id, + user_id='test_user', + app_conversation_service=mock_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 (should return True immediately since conversation exists) + assert result is True + + # Verify that get_app_conversation was called during the synchronous check + mock_service.get_app_conversation.assert_called_once_with(conversation_uuid) + + # Execute the background task that was scheduled + assert len(background_tasks.tasks) == 1 + task_func, task_args, task_kwargs = background_tasks.tasks[0] + await task_func(*task_args, **task_kwargs) + + # Verify that delete_app_conversation was called in the background task + mock_service.delete_app_conversation.assert_called_once_with(conversation_uuid) + + # Verify that delete_sandbox was called in the background task + mock_sandbox_service.delete_sandbox.assert_called_once_with('test-sandbox-id') @pytest.mark.asyncio @@ -1029,21 +1065,37 @@ async def test_delete_v1_conversation_not_found(): mock_service.get_app_conversation = AsyncMock(return_value=None) mock_service.delete_app_conversation = AsyncMock(return_value=False) - # 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 sandbox service + with patch( + 'openhands.server.routes.manage_conversations.sandbox_service_dependency' + ) as mock_sandbox_dep: + mock_sandbox_service = MagicMock() + mock_sandbox_dep.return_value = mock_sandbox_service - # Verify the result - assert result is False + # Create a mock BackgroundTasks + from fastapi import BackgroundTasks + background_tasks = BackgroundTasks() - # 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( + background_tasks=background_tasks, + conversation_id=conversation_id, + user_id='test_user', + app_conversation_service=mock_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 (should be False since conversation doesn't exist) + assert result is False + + # Verify that get_app_conversation was called during the synchronous check + mock_service.get_app_conversation.assert_called_once_with(conversation_uuid) + + # Verify that delete_app_conversation was NOT called + mock_service.delete_app_conversation.assert_not_called() + + # Verify that no background tasks were scheduled + assert len(background_tasks.tasks) == 0 @pytest.mark.asyncio @@ -1091,19 +1143,37 @@ 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 sandbox service + with patch( + 'openhands.server.routes.manage_conversations.sandbox_service_dependency' + ) as mock_sandbox_dep: + mock_sandbox_service = MagicMock() + mock_sandbox_dep.return_value = mock_sandbox_service - # Verify the result - assert result is True + # Create a mock BackgroundTasks + from fastapi import BackgroundTasks + background_tasks = BackgroundTasks() - # 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( + background_tasks=background_tasks, + conversation_id=conversation_id, + user_id='test_user', + app_conversation_service=mock_service, + sandbox_service=mock_sandbox_service, + ) + + # Verify the result (should be True since V0 conversation exists) + assert result is True + + # Execute the background task that was scheduled + assert len(background_tasks.tasks) == 1 + task_func, task_args, task_kwargs = background_tasks.tasks[0] + await task_func(*task_args, **task_kwargs) + + # 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 @@ -1159,19 +1229,37 @@ async def test_delete_v1_conversation_service_error(): 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 sandbox service + with patch( + 'openhands.server.routes.manage_conversations.sandbox_service_dependency' + ) as mock_sandbox_dep: + mock_sandbox_service = MagicMock() + mock_sandbox_dep.return_value = mock_sandbox_service - # Verify the result (should fallback to V0) - assert result is True + # Create a mock BackgroundTasks + from fastapi import BackgroundTasks + background_tasks = BackgroundTasks() - # 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( + background_tasks=background_tasks, + conversation_id=conversation_id, + user_id='test_user', + app_conversation_service=mock_service, + sandbox_service=mock_sandbox_service, + ) + + # Verify the result (should fallback to V0) + assert result is True + + # Execute the background task that was scheduled + assert len(background_tasks.tasks) == 1 + task_func, task_args, task_kwargs = background_tasks.tasks[0] + await task_func(*task_args, **task_kwargs) + + # 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 @@ -1216,21 +1304,43 @@ async def test_delete_v1_conversation_with_agent_server(): ) mock_service.delete_app_conversation = AsyncMock(return_value=True) - # 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 sandbox service + with patch( + 'openhands.server.routes.manage_conversations.sandbox_service_dependency' + ) as mock_sandbox_dep: + mock_sandbox_service = MagicMock() + mock_sandbox_service.delete_sandbox = AsyncMock() + mock_sandbox_dep.return_value = mock_sandbox_service - # Verify the result - assert result is True + # Create a mock BackgroundTasks + from fastapi import BackgroundTasks + background_tasks = BackgroundTasks() - # 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( + background_tasks=background_tasks, + conversation_id=conversation_id, + user_id='test_user', + app_conversation_service=mock_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 (should return True immediately since conversation exists) + assert result is True + + # Verify that get_app_conversation was called during the synchronous check + mock_service.get_app_conversation.assert_called_once_with(conversation_uuid) + + # Execute the background task that was scheduled + assert len(background_tasks.tasks) == 1 + task_func, task_args, task_kwargs = background_tasks.tasks[0] + await task_func(*task_args, **task_kwargs) + + # Verify that delete_app_conversation was called in the background task + mock_service.delete_app_conversation.assert_called_once_with(conversation_uuid) + + # Verify that delete_sandbox was called in the background task + mock_sandbox_service.delete_sandbox.assert_called_once_with('test-sandbox-id') @pytest.mark.asyncio