mirror of
https://github.com/OpenHands/OpenHands.git
synced 2026-10-07 16:08:23 +08:00
fix(app-server): write titles column-specifically to avoid clobbering concurrent updates
The title poller held a conversation snapshot for up to ~12s and then full-row-saved it, silently erasing any stats/metadata updates that landed in between. Add update_title and use it in the processor.
This commit is contained in:
@@ -116,6 +116,14 @@ class AppConversationInfoService(ABC):
|
||||
conversation_id: The ID of the conversation to update
|
||||
"""
|
||||
|
||||
@abstractmethod
|
||||
async def update_title(
|
||||
self,
|
||||
conversation_id: UUID,
|
||||
title: str,
|
||||
) -> None:
|
||||
"""Update only the title for a conversation (no full-row rewrite)."""
|
||||
|
||||
@abstractmethod
|
||||
async def update_execution_status(
|
||||
self,
|
||||
|
||||
@@ -609,6 +609,35 @@ class SQLAppConversationInfoService(AppConversationInfoService):
|
||||
stack_info=True,
|
||||
)
|
||||
|
||||
async def update_title(
|
||||
self,
|
||||
conversation_id: UUID,
|
||||
title: str,
|
||||
) -> None:
|
||||
"""Column-specific title write.
|
||||
|
||||
The title poller holds a stale snapshot for many seconds; a full-row
|
||||
save from it would clobber concurrently updated fields (metrics,
|
||||
llm_model), so only the title column is touched.
|
||||
"""
|
||||
query = await self._secure_select()
|
||||
query = query.where(
|
||||
StoredConversationMetadata.conversation_id == str(conversation_id)
|
||||
)
|
||||
result = await self.db_session.execute(query)
|
||||
stored = result.scalar_one_or_none()
|
||||
|
||||
if not stored:
|
||||
logger.debug(
|
||||
'Conversation %s not found or not accessible, skipping title update',
|
||||
conversation_id,
|
||||
)
|
||||
return
|
||||
|
||||
stored.title = title
|
||||
stored.last_updated_at = utc_now()
|
||||
await self.db_session.commit()
|
||||
|
||||
async def update_execution_status(
|
||||
self,
|
||||
conversation_id: UUID,
|
||||
|
||||
@@ -5,9 +5,6 @@ from uuid import UUID
|
||||
|
||||
import httpx
|
||||
|
||||
from openhands.app_server.app_conversation.app_conversation_models import (
|
||||
AppConversationInfo,
|
||||
)
|
||||
from openhands.app_server.event_callback.event_callback_models import (
|
||||
EventCallback,
|
||||
EventCallbackProcessor,
|
||||
@@ -137,15 +134,10 @@ class SetTitleCallbackProcessor(EventCallbackProcessor):
|
||||
)
|
||||
return None
|
||||
|
||||
# Save the conversation info
|
||||
info = AppConversationInfo(
|
||||
**{
|
||||
name: getattr(app_conversation, name)
|
||||
for name in AppConversationInfo.model_fields
|
||||
}
|
||||
)
|
||||
info.title = title
|
||||
await app_conversation_info_service.save_app_conversation_info(info)
|
||||
# Column-specific write: the snapshot fetched before the poll is
|
||||
# stale, and a full-row save would clobber concurrent updates
|
||||
# (metrics, llm_model).
|
||||
await app_conversation_info_service.update_title(conversation_id, title)
|
||||
|
||||
# Disable callback - we have already set the status
|
||||
callback.status = EventCallbackStatus.DISABLED
|
||||
|
||||
@@ -124,11 +124,11 @@ async def test_set_title_callback_processor_fetches_title_from_conversation():
|
||||
assert httpx_client.calls[0][0] == expected_url
|
||||
assert httpx_client.calls[0][1] == {'X-Session-API-Key': session_api_key}
|
||||
|
||||
app_conversation_info_service.save_app_conversation_info.assert_called_once()
|
||||
saved_info = app_conversation_info_service.save_app_conversation_info.call_args[0][
|
||||
0
|
||||
]
|
||||
assert saved_info.title == 'Generated Title'
|
||||
# Column-specific write — the stale snapshot must never be re-saved.
|
||||
app_conversation_info_service.update_title.assert_called_once_with(
|
||||
conversation_id, 'Generated Title'
|
||||
)
|
||||
app_conversation_info_service.save_app_conversation_info.assert_not_called()
|
||||
|
||||
assert callback.status == EventCallbackStatus.DISABLED
|
||||
event_callback_service.save_event_callback.assert_called_once()
|
||||
@@ -204,6 +204,7 @@ async def test_set_title_callback_processor_no_title_yet_returns_none():
|
||||
assert result is None
|
||||
|
||||
app_conversation_info_service.save_app_conversation_info.assert_not_called()
|
||||
app_conversation_info_service.update_title.assert_not_called()
|
||||
event_callback_service.save_event_callback.assert_not_called()
|
||||
assert callback.status == EventCallbackStatus.ACTIVE
|
||||
|
||||
@@ -290,5 +291,6 @@ async def test_set_title_callback_processor_request_errors_return_none():
|
||||
assert len(httpx_client.calls) == 4
|
||||
assert logger_warning.call_count == 4
|
||||
app_conversation_info_service.save_app_conversation_info.assert_not_called()
|
||||
app_conversation_info_service.update_title.assert_not_called()
|
||||
event_callback_service.save_event_callback.assert_not_called()
|
||||
assert callback.status == EventCallbackStatus.ACTIVE
|
||||
|
||||
@@ -0,0 +1,66 @@
|
||||
"""Tests for the column-specific title update."""
|
||||
|
||||
from typing import AsyncGenerator
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker, create_async_engine
|
||||
from sqlalchemy.pool import StaticPool
|
||||
|
||||
from openhands.app_server.app_conversation.sql_app_conversation_info_service import (
|
||||
SQLAppConversationInfoService,
|
||||
StoredConversationMetadata,
|
||||
)
|
||||
from openhands.app_server.user.specifiy_user_context import SpecifyUserContext
|
||||
from openhands.app_server.utils.sql_utils import Base
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
async def async_session() -> AsyncGenerator[AsyncSession, None]:
|
||||
engine = create_async_engine(
|
||||
'sqlite+aiosqlite:///:memory:',
|
||||
poolclass=StaticPool,
|
||||
connect_args={'check_same_thread': False},
|
||||
)
|
||||
async with engine.begin() as conn:
|
||||
await conn.run_sync(Base.metadata.create_all)
|
||||
maker = async_sessionmaker(engine, class_=AsyncSession, expire_on_commit=False)
|
||||
async with maker() as session:
|
||||
yield session
|
||||
await engine.dispose()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_title_leaves_other_columns_alone(async_session):
|
||||
conversation_id = uuid4()
|
||||
stored = StoredConversationMetadata(
|
||||
conversation_id=str(conversation_id),
|
||||
conversation_version='V1',
|
||||
title='stub',
|
||||
llm_model='litellm_proxy/gpt-5.5',
|
||||
accumulated_cost=0.25,
|
||||
prompt_tokens=300,
|
||||
completion_tokens=30,
|
||||
)
|
||||
async_session.add(stored)
|
||||
await async_session.commit()
|
||||
|
||||
service = SQLAppConversationInfoService(
|
||||
db_session=async_session, user_context=SpecifyUserContext(user_id=None)
|
||||
)
|
||||
await service.update_title(conversation_id, 'Generated Title')
|
||||
|
||||
await async_session.refresh(stored)
|
||||
assert stored.title == 'Generated Title'
|
||||
assert stored.llm_model == 'litellm_proxy/gpt-5.5'
|
||||
assert stored.accumulated_cost == pytest.approx(0.25)
|
||||
assert stored.prompt_tokens == 300
|
||||
assert stored.completion_tokens == 30
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_title_missing_conversation_is_noop(async_session):
|
||||
service = SQLAppConversationInfoService(
|
||||
db_session=async_session, user_context=SpecifyUserContext(user_id=None)
|
||||
)
|
||||
await service.update_title(uuid4(), 'whatever')
|
||||
@@ -10,6 +10,15 @@ resolution-markers = [
|
||||
"python_full_version < '3.13' and sys_platform != 'emscripten' and sys_platform != 'win32'",
|
||||
]
|
||||
|
||||
[options]
|
||||
exclude-newer = "2026-07-15T18:06:03.997523Z"
|
||||
exclude-newer-span = "P7D"
|
||||
|
||||
[options.exclude-newer-package]
|
||||
openhands-tools = false
|
||||
openhands-agent-server = false
|
||||
openhands-sdk = false
|
||||
|
||||
[[package]]
|
||||
name = "agent-client-protocol"
|
||||
version = "0.10.1"
|
||||
@@ -744,7 +753,7 @@ name = "clr-loader"
|
||||
version = "0.2.10"
|
||||
source = { registry = "https://pypi.org/simple" }
|
||||
dependencies = [
|
||||
{ name = "cffi" },
|
||||
{ name = "cffi", marker = "sys_platform == 'win32'" },
|
||||
]
|
||||
sdist = { url = "https://files.pythonhosted.org/packages/18/24/c12faf3f61614b3131b5c98d3bf0d376b49c7feaa73edca559aeb2aee080/clr_loader-0.2.10.tar.gz", hash = "sha256:81f114afbc5005bafc5efe5af1341d400e22137e275b042a8979f3feb9fc9446", size = 83605, upload-time = "2026-01-03T23:13:06.984Z" }
|
||||
wheels = [
|
||||
@@ -7135,7 +7144,7 @@ name = "pythonnet"
|
||||
version = "3.0.5"
|
||||
source = { registry = "https://pypi.org/simple" }
|
||||
dependencies = [
|
||||
{ name = "clr-loader" },
|
||||
{ name = "clr-loader", marker = "sys_platform == 'win32'" },
|
||||
]
|
||||
sdist = { url = "https://files.pythonhosted.org/packages/9a/d6/1afd75edd932306ae9bd2c2d961d603dc2b52fcec51b04afea464f1f6646/pythonnet-3.0.5.tar.gz", hash = "sha256:48e43ca463941b3608b32b4e236db92d8d40db4c58a75ace902985f76dac21cf", size = 239212, upload-time = "2024-12-13T08:30:44.393Z" }
|
||||
wheels = [
|
||||
@@ -7622,8 +7631,8 @@ name = "secretstorage"
|
||||
version = "3.5.0"
|
||||
source = { registry = "https://pypi.org/simple" }
|
||||
dependencies = [
|
||||
{ name = "cryptography" },
|
||||
{ name = "jeepney" },
|
||||
{ name = "cryptography", marker = "sys_platform != 'emscripten' and sys_platform != 'win32'" },
|
||||
{ name = "jeepney", marker = "sys_platform != 'emscripten' and sys_platform != 'win32'" },
|
||||
]
|
||||
sdist = { url = "https://files.pythonhosted.org/packages/1c/03/e834bcd866f2f8a49a85eaff47340affa3bfa391ee9912a952a1faa68c7b/secretstorage-3.5.0.tar.gz", hash = "sha256:f04b8e4689cbce351744d5537bf6b1329c6fc68f91fa666f60a380edddcd11be", size = 19884, upload-time = "2025-11-23T19:02:53.191Z" }
|
||||
wheels = [
|
||||
@@ -8410,7 +8419,7 @@ name = "xattr"
|
||||
version = "1.3.0"
|
||||
source = { registry = "https://pypi.org/simple" }
|
||||
dependencies = [
|
||||
{ name = "cffi" },
|
||||
{ name = "cffi", marker = "sys_platform != 'emscripten' and sys_platform != 'win32'" },
|
||||
]
|
||||
sdist = { url = "https://files.pythonhosted.org/packages/08/d5/25f7b19af3a2cb4000cac4f9e5525a40bec79f4f5d0ac9b517c0544586a0/xattr-1.3.0.tar.gz", hash = "sha256:30439fabd7de0787b27e9a6e1d569c5959854cb322f64ce7380fedbfa5035036", size = 17148, upload-time = "2025-10-13T22:16:47.353Z" }
|
||||
wheels = [
|
||||
|
||||
Reference in New Issue
Block a user