github/OpenHands
1
0
Fork 0
mirror of https://github.com/OpenHands/OpenHands.git synced 2025-12-26 05:48:36 +08:00
Code Issues Packages Projects Releases Wiki Activity

Fix unit tests for background delete conversation process

Browse Source 
- 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 <openhands@all-hands.dev>
This commit is contained in:
openhands 2025-11-14 12:42:33 +00:00
parent 7263657937
commit bb064e9907
2 changed files with 257 additions and 85 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

116
openhands/server/routes/manage_conversations.py
View File

@ -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:

226
tests/unit/server/data_models/test_conversation.py
View File

@ -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