refactor: update delete_app_conversation to accept ID instead of object (#11486)

Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: Tim O'Farrell <tofarr@gmail.com>

openhands/app_server/app_conversation/app_conversation_info_service.py

@@ -57,6 +57,16 @@ class AppConversationInfoService(ABC):
             ]
         )
 
+    @abstractmethod
+    async def delete_app_conversation_info(self, conversation_id: UUID) -> bool:
+        """Delete a conversation info from the database.
+
+        Args:
+            conversation_id: The ID of the conversation to delete.
+
+        Returns True if the conversation was deleted successfully, False otherwise.
+        """
+
     # Mutators
 
     @abstractmethod

openhands/app_server/app_conversation/app_conversation_service.py

@@ -95,6 +95,21 @@ class AppConversationService(ABC):
         """Run the setup scripts for the project and yield status updates"""
         yield task
 
+    @abstractmethod
+    async def delete_app_conversation(self, conversation_id: UUID) -> bool:
+        """Delete a V1 conversation and all its associated data.
+
+        Args:
+            conversation_id: The UUID of the conversation to delete.
+
+        This method should:
+        1. Delete the conversation from the database
+        2. Call the agent server to delete the conversation
+        3. Clean up any related data
+
+        Returns True if the conversation was deleted successfully, False otherwise.
+        """
+
 
 class AppConversationServiceInjector(
     DiscriminatedUnionMixin, Injector[AppConversationService], ABC

openhands/app_server/app_conversation/app_conversation_start_task_service.py

@@ -56,6 +56,16 @@ class AppConversationStartTaskService(ABC):
         Return the stored task
         """
 
+    @abstractmethod
+    async def delete_app_conversation_start_tasks(self, conversation_id: UUID) -> bool:
+        """Delete all start tasks associated with a conversation.
+
+        Args:
+            conversation_id: The ID of the conversation to delete tasks for.
+
+        Returns True if any tasks were deleted successfully, False otherwise.
+        """
+
 
 class AppConversationStartTaskServiceInjector(
     DiscriminatedUnionMixin, Injector[AppConversationStartTaskService], ABC

openhands/app_server/app_conversation/live_status_app_conversation_service.py

@@ -39,6 +39,9 @@ from openhands.app_server.app_conversation.app_conversation_start_task_service i
 from openhands.app_server.app_conversation.git_app_conversation_service import (
     GitAppConversationService,
 )
+from openhands.app_server.app_conversation.sql_app_conversation_info_service import (
+    SQLAppConversationInfoService,
+)
 from openhands.app_server.errors import SandboxError
 from openhands.app_server.sandbox.docker_sandbox_service import DockerSandboxService
 from openhands.app_server.sandbox.sandbox_models import (
@@ -529,6 +532,101 @@ class LiveStatusAppConversationService(GitAppConversationService):
             f'Successfully updated agent-server conversation {conversation_id} title to "{new_title}"'
         )
 
+    async def delete_app_conversation(self, conversation_id: UUID) -> bool:
+        """Delete a V1 conversation and all its associated data.
+
+        Args:
+            conversation_id: The UUID of the conversation to delete.
+        """
+        # Check if we have the required SQL implementation for transactional deletion
+        if not isinstance(
+            self.app_conversation_info_service, SQLAppConversationInfoService
+        ):
+            _logger.error(
+                f'Cannot delete V1 conversation {conversation_id}: SQL implementation required for transactional deletion',
+                extra={'conversation_id': str(conversation_id)},
+            )
+            return False
+
+        try:
+            # First, fetch the conversation to get the full object needed for agent server deletion
+            app_conversation = await self.get_app_conversation(conversation_id)
+            if not app_conversation:
+                _logger.warning(
+                    f'V1 conversation {conversation_id} not found for deletion',
+                    extra={'conversation_id': str(conversation_id)},
+                )
+                return False
+
+            # Delete from agent server if sandbox is running
+            await self._delete_from_agent_server(app_conversation)
+
+            # Delete from database using the conversation info from app_conversation
+            # AppConversation extends AppConversationInfo, so we can use it directly
+            return await self._delete_from_database(app_conversation)
+
+        except Exception as e:
+            _logger.error(
+                f'Error deleting V1 conversation {conversation_id}: {e}',
+                extra={'conversation_id': str(conversation_id)},
+                exc_info=True,
+            )
+            return False
+
+    async def _delete_from_agent_server(
+        self, app_conversation: AppConversation
+    ) -> None:
+        """Delete conversation from agent server if sandbox is running."""
+        conversation_id = app_conversation.id
+        if not (
+            app_conversation.sandbox_status == SandboxStatus.RUNNING
+            and app_conversation.session_api_key
+        ):
+            return
+
+        try:
+            # Get sandbox info to find agent server URL
+            sandbox = await self.sandbox_service.get_sandbox(
+                app_conversation.sandbox_id
+            )
+            if sandbox and sandbox.exposed_urls:
+                agent_server_url = self._get_agent_server_url(sandbox)
+
+                # Call agent server delete API
+                response = await self.httpx_client.delete(
+                    f'{agent_server_url}/api/conversations/{conversation_id}',
+                    headers={'X-Session-API-Key': app_conversation.session_api_key},
+                    timeout=30.0,
+                )
+                response.raise_for_status()
+        except Exception as e:
+            _logger.warning(
+                f'Failed to delete conversation from agent server: {e}',
+                extra={'conversation_id': str(conversation_id)},
+            )
+            # Continue with database cleanup even if agent server call fails
+
+    async def _delete_from_database(
+        self, app_conversation_info: AppConversationInfo
+    ) -> bool:
+        """Delete conversation from database.
+
+        Args:
+            app_conversation_info: The app conversation info to delete (already fetched).
+        """
+        # The session is already managed by the dependency injection system
+        # No need for explicit transaction management here
+        deleted_info = (
+            await self.app_conversation_info_service.delete_app_conversation_info(
+                app_conversation_info.id
+            )
+        )
+        deleted_tasks = await self.app_conversation_start_task_service.delete_app_conversation_start_tasks(
+            app_conversation_info.id
+        )
+
+        return deleted_info or deleted_tasks
+
 
 class LiveStatusAppConversationServiceInjector(AppConversationServiceInjector):
     sandbox_startup_timeout: int = Field(

openhands/app_server/app_conversation/sql_app_conversation_info_service.py

@@ -356,9 +356,9 @@ class SQLAppConversationInfoService(AppConversationInfoService):
             sandbox_id=stored.sandbox_id,
             selected_repository=stored.selected_repository,
             selected_branch=stored.selected_branch,
-            git_provider=ProviderType(stored.git_provider)
-            if stored.git_provider
-            else None,
+            git_provider=(
+                ProviderType(stored.git_provider) if stored.git_provider else None
+            ),
             title=stored.title,
             trigger=ConversationTrigger(stored.trigger) if stored.trigger else None,
             pr_number=stored.pr_number,
@@ -375,6 +375,34 @@ class SQLAppConversationInfoService(AppConversationInfoService):
             value = value.replace(tzinfo=UTC)
         return value
 
+    async def delete_app_conversation_info(self, conversation_id: UUID) -> bool:
+        """Delete a conversation info from the database.
+
+        Args:
+            conversation_id: The ID of the conversation to delete.
+
+        Returns True if the conversation was deleted successfully, False otherwise.
+        """
+        from sqlalchemy import delete
+
+        # Build secure delete query with user context filtering
+        delete_query = delete(StoredConversationMetadata).where(
+            StoredConversationMetadata.conversation_id == str(conversation_id)
+        )
+
+        # Apply user security filtering - only allow deletion of conversations owned by the current user
+        user_id = await self.user_context.get_user_id()
+        if user_id:
+            delete_query = delete_query.where(
+                StoredConversationMetadata.user_id == user_id
+            )
+
+        # Execute the secure delete query
+        result = await self.db_session.execute(delete_query)
+        await self.db_session.commit()
+
+        return result.rowcount > 0
+
 
 class SQLAppConversationInfoServiceInjector(AppConversationInfoServiceInjector):
     async def inject(

openhands/app_server/app_conversation/sql_app_conversation_start_task_service.py

@@ -180,9 +180,11 @@ class SQLAppConversationStartTaskService(AppConversationStartTaskService):
 
         # Return tasks in the same order as requested, with None for missing ones
         return [
-            AppConversationStartTask(**row2dict(tasks_by_id[task_id]))
-            if task_id in tasks_by_id
-            else None
+            (
+                AppConversationStartTask(**row2dict(tasks_by_id[task_id]))
+                if task_id in tasks_by_id
+                else None
+            )
             for task_id in task_ids
         ]
 
@@ -219,6 +221,29 @@ class SQLAppConversationStartTaskService(AppConversationStartTaskService):
         await self.session.commit()
         return task
 
+    async def delete_app_conversation_start_tasks(self, conversation_id: UUID) -> bool:
+        """Delete all start tasks associated with a conversation.
+
+        Args:
+            conversation_id: The ID of the conversation to delete tasks for.
+        """
+        from sqlalchemy import delete
+
+        # Build secure delete query with user filter if user_id is set
+        delete_query = delete(StoredAppConversationStartTask).where(
+            StoredAppConversationStartTask.app_conversation_id == conversation_id
+        )
+
+        if self.user_id:
+            delete_query = delete_query.where(
+                StoredAppConversationStartTask.created_by_user_id == self.user_id
+            )
+
+        result = await self.session.execute(delete_query)
+
+        # Return True if any rows were affected
+        return result.rowcount > 0
+
 
 class SQLAppConversationStartTaskServiceInjector(
     AppConversationStartTaskServiceInjector

openhands/server/routes/manage_conversations.py

@@ -465,15 +465,59 @@ async def get_conversation(
 async def delete_conversation(
     conversation_id: str = Depends(validate_conversation_id),
     user_id: str | None = Depends(get_user_id),
+    app_conversation_service: AppConversationService = app_conversation_service_dependency,
 ) -> bool:
+    # Try V1 conversation first
+    v1_result = await _try_delete_v1_conversation(
+        conversation_id, app_conversation_service
+    )
+    if v1_result is not None:
+        return v1_result
+
+    # V0 conversation logic
+    return await _delete_v0_conversation(conversation_id, user_id)
+
+
+async def _try_delete_v1_conversation(
+    conversation_id: str, app_conversation_service: AppConversationService
+) -> bool | None:
+    """Try to delete a V1 conversation. Returns None if not a V1 conversation."""
+    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
+            return await app_conversation_service.delete_app_conversation(
+                app_conversation.id
+            )
+    except (ValueError, TypeError):
+        # Not a valid UUID, continue with V0 logic
+        pass
+    except Exception:
+        # Some other error, continue with V0 logic
+        pass
+
+    return None
+
+
+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)
     try:
         await conversation_store.get_metadata(conversation_id)
     except FileNotFoundError:
         return False
+
+    # Stop the conversation if it's running
     is_running = await conversation_manager.is_agent_loop_running(conversation_id)
     if is_running:
         await conversation_manager.close_session(conversation_id)
+
+    # Clean up runtime and metadata
     runtime_cls = get_runtime_cls(config.runtime)
     await runtime_cls.delete(conversation_id)
     await conversation_store.delete_metadata(conversation_id)

tests/unit/server/data_models/test_conversation.py

@@ -909,6 +909,12 @@ async def test_delete_conversation():
             # Return the mock store from get_instance
             mock_get_instance.return_value = mock_store
 
+            # Create a mock app conversation service
+            mock_app_conversation_service = MagicMock()
+            mock_app_conversation_service.get_app_conversation = AsyncMock(
+                return_value=None
+            )
+
             # Mock the conversation manager
             with patch(
                 'openhands.server.routes.manage_conversations.conversation_manager'
@@ -926,7 +932,9 @@ async def test_delete_conversation():
 
                     # Call delete_conversation
                     result = await delete_conversation(
-                        'some_conversation_id', user_id='12345'
+                        conversation_id='some_conversation_id',
+                        user_id='12345',
+                        app_conversation_service=mock_app_conversation_service,
                     )
 
                     # Verify the result
@@ -943,6 +951,288 @@ async def test_delete_conversation():
                     )
 
 
+@pytest.mark.asyncio
+async def test_delete_v1_conversation_success():
+    """Test successful deletion of a V1 conversation."""
+    from uuid import uuid4
+
+    from openhands.app_server.app_conversation.app_conversation_models import (
+        AppConversation,
+    )
+    from openhands.app_server.sandbox.sandbox_models import SandboxStatus
+    from openhands.sdk.conversation.state import AgentExecutionStatus
+
+    conversation_uuid = uuid4()
+    conversation_id = str(conversation_uuid)
+
+    # Mock the app conversation service
+    with patch(
+        'openhands.server.routes.manage_conversations.app_conversation_service_dependency'
+    ) as mock_service_dep:
+        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,
+            agent_status=AgentExecutionStatus.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)
+
+        # Call delete_conversation with V1 conversation ID
+        result = await delete_conversation(
+            conversation_id=conversation_id,
+            user_id='test_user',
+            app_conversation_service=mock_service,
+        )
+
+        # Verify the result
+        assert result is True
+
+        # Verify that get_app_conversation was called
+        mock_service.get_app_conversation.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
+async def test_delete_v1_conversation_not_found():
+    """Test deletion of a V1 conversation that doesn't exist."""
+    from uuid import uuid4
+
+    conversation_uuid = uuid4()
+    conversation_id = str(conversation_uuid)
+
+    # Mock the app conversation service
+    with patch(
+        'openhands.server.routes.manage_conversations.app_conversation_service_dependency'
+    ) as mock_service_dep:
+        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)
+
+        # Call delete_conversation with V1 conversation ID
+        result = await delete_conversation(
+            conversation_id=conversation_id,
+            user_id='test_user',
+            app_conversation_service=mock_service,
+        )
+
+        # Verify the result
+        assert result is False
+
+        # Verify that get_app_conversation was called
+        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()
+
+
+@pytest.mark.asyncio
+async def test_delete_v1_conversation_invalid_uuid():
+    """Test deletion with invalid UUID falls back to V0 logic."""
+    conversation_id = 'invalid-uuid-format'
+
+    # Mock the app conversation service
+    with patch(
+        'openhands.server.routes.manage_conversations.app_conversation_service_dependency'
+    ) as mock_service_dep:
+        mock_service = MagicMock()
+        mock_service_dep.return_value = mock_service
+
+        # Mock V0 conversation logic
+        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
+
+            # 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={})
+
+                # 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(
+                        conversation_id=conversation_id,
+                        user_id='test_user',
+                        app_conversation_service=mock_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
+async def test_delete_v1_conversation_service_error():
+    """Test deletion when app conversation service raises an error."""
+    from uuid import uuid4
+
+    conversation_uuid = uuid4()
+    conversation_id = str(conversation_uuid)
+
+    # Mock the app conversation service
+    with patch(
+        'openhands.server.routes.manage_conversations.app_conversation_service_dependency'
+    ) as mock_service_dep:
+        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
+        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
+
+            # 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={})
+
+                # 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(
+                        conversation_id=conversation_id,
+                        user_id='test_user',
+                        app_conversation_service=mock_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
+async def test_delete_v1_conversation_with_agent_server():
+    """Test V1 conversation deletion with agent server integration."""
+    from uuid import uuid4
+
+    from openhands.app_server.app_conversation.app_conversation_models import (
+        AppConversation,
+    )
+    from openhands.app_server.sandbox.sandbox_models import SandboxStatus
+    from openhands.sdk.conversation.state import AgentExecutionStatus
+
+    conversation_uuid = uuid4()
+    conversation_id = str(conversation_uuid)
+
+    # Mock the app conversation service
+    with patch(
+        'openhands.server.routes.manage_conversations.app_conversation_service_dependency'
+    ) as mock_service_dep:
+        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,
+            agent_status=AgentExecutionStatus.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)
+
+        # Call delete_conversation with V1 conversation ID
+        result = await delete_conversation(
+            conversation_id=conversation_id,
+            user_id='test_user',
+            app_conversation_service=mock_service,
+        )
+
+        # Verify the result
+        assert result is True
+
+        # Verify that get_app_conversation was called
+        mock_service.get_app_conversation.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
 async def test_new_conversation_with_bearer_auth(provider_handler_mock):
     """Test creating a new conversation with bearer authentication."""