LCORE-1166: Rebased conversation history changes #1129
Conversation
WalkthroughEndpoints now capture a UTC Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Endpoint as QueryEndpoint
participant Storage as UtilsQuery
participant DB as Database
Client->>Endpoint: Send query request
Endpoint->>Endpoint: Process request / produce response
Endpoint->>Endpoint: Capture completed_at (UTC)
Endpoint->>Storage: store_query_results(model, completed_at, ...)
Storage->>Storage: extract_provider_and_model_from_model_id(model)
Storage->>DB: Acquire FOR UPDATE lock on UserTurn rows
DB-->>Storage: Lock acquired
Storage->>DB: Query max(turn_number) for conversation
DB-->>Storage: Return max turn_number
Storage->>DB: Insert new UserTurn (turn_number, started_at, completed_at, provider_id, model_id)
DB-->>Storage: UserTurn inserted
Storage->>DB: Persist conversation metadata (last_used_model, started_at, completed_at)
DB-->>Storage: Persisted
Storage-->>Endpoint: Done
Endpoint-->>Client: Return final response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/query.py (1)
413-420:⚠️ Potential issue | 🟠 MajorBug:
completed_atparameter is overwritten before building the cache entry, causing DB/cache timestamp inconsistency.Line 413 reassigns
completed_atwith a freshly computed timestamp, shadowing the function parameter received at line 336. The database record (viapersist_user_conversation_detailsat line 401) uses the original caller-providedcompleted_at, but theCacheEntryat line 420 uses this new value. This creates an inconsistency between the persisted database record and the cache.🐛 Proposed fix — remove the redundant reassignment
# Store conversation in cache try: - completed_at = datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%SZ") cache_entry = CacheEntry( query=query_request.query, response=summary.llm_response,
🤖 Fix all issues with AI agents
In `@tests/unit/app/endpoints/test_conversations.py`:
- Around line 799-802: The tests in TestGetConversationEndpoint are patching the
wrong function: replace any mocker.patch targeting "retrieve_conversation" with
"validate_and_retrieve_conversation" so the actual call from
get_conversation_endpoint_handler is intercepted; update mocks in
test_llama_stack_not_found_error, test_get_conversation_forbidden, and
test_sqlalchemy_error_in_get_conversation to patch
validate_and_retrieve_conversation (and keep returning mock_conversation or
raising the intended exceptions) so the tests exercise the correct code path
invoked by get_conversation_endpoint_handler.
🧹 Nitpick comments (5)
tests/unit/app/endpoints/test_conversations.py (1)
827-870: New APIStatusError test looks good overall.The test correctly verifies that an
APIStatusErrorduring item retrieval results in an HTTP 404 with the expected detail payload.One minor note: line 841 patches
can_access_conversation, but sincevalidate_and_retrieve_conversationis fully mocked (line 842-844), the access check may never be reached. If so, the patch is dead weight — consider removing it for clarity, or confirm it's needed by the endpoint's call order.tests/unit/utils/test_query.py (2)
527-528:@pytest.mark.asyncioon a synchronous function is unnecessary.
persist_user_conversation_detailsis a synchronous function (def, notasync def), so@pytest.mark.asyncioandasync defon these tests are not needed. While it won't break anything, it adds unnecessary overhead and is misleading.♻️ Suggested fix
- `@pytest.mark.asyncio` - async def test_create_new_conversation(self, mocker: MockerFixture) -> None: + def test_create_new_conversation(self, mocker: MockerFixture) -> None:Same applies to
test_update_existing_conversationat line 576–577.
547-556: Deadif not argsbranch inquery_side_effect.The
if not args:branch (line 549) is unreachable —session.query(func.max(UserTurn.turn_number))always passes one argument. Thefunc.max(...)expression won't matchUserConversationorUserTurn, so it correctly falls through to the defaultreturn mock_max_queryat line 556. Thenot argscheck is harmless but dead code.♻️ Suggested simplification
def query_side_effect(*args: Any) -> Any: """Route queries based on the argument type.""" - if not args: - return mock_max_query - arg = args[0] - if arg is UserConversation: + if args and args[0] is UserConversation: return mock_conv_query - if arg is UserTurn: + if args and args[0] is UserTurn: return mock_turn_lock_query return mock_max_querySame applies to the duplicate at lines 603–612.
src/utils/query.py (2)
331-341: Docstring parameter name inconsistency:modelparameter description says "model identifier" but it actually expects the compositeprovider/modelformat.The callers pass
responses_params.modelwhich is in"provider/model"format (as evidenced byextract_provider_and_model_from_model_id(model)at line 366). The docstring at line 354 should clarify this expected format to avoid confusion for future callers.📝 Suggested docstring improvement
- model: The model identifier + model: The full model identifier in "provider/model" format
548-553: Combine the lock and max() query to reduce unnecessary memory consumption.The current implementation fetches all
UserTurnrows into memory just to acquire locks, which is wasteful for conversations with many turns. Since the rows are never read—only the lock matters—combine the locking and aggregation into a single query:♻️ Suggested refactor
- # Lock UserTurn rows for this conversation to prevent race conditions - # when computing max(turn_number) and inserting a new turn - session.query(UserTurn).filter_by( - conversation_id=normalized_id - ).with_for_update().all() - # Recompute max(turn_number) after acquiring the lock - max_turn_number = ( - session.query(func.max(UserTurn.turn_number)) - .filter_by(conversation_id=normalized_id) - .scalar() - ) + # Lock and compute max turn number in a single query + max_turn_number = ( + session.query(func.max(UserTurn.turn_number)) + .filter_by(conversation_id=normalized_id) + .with_for_update() + .scalar() + )Note: PostgreSQL properly locks all rows scanned by the aggregate. SQLite silently ignores
FOR UPDATE(as with the current code), so behavior is unchanged. Verify the lock semantics against your supported databases in testing.
asimurka
force-pushed
the
rebase_conversation_history
branch
from
a0ef91f to
5c354fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/utils/query.py`:
- Around line 547-554: The .with_for_update() call when computing max turn
number on UserTurn (the
session.query(...).filter_by(conversation_id=normalized_id).with_for_update().scalar()
block) is invalid on SQLite; guard it by checking the current DB dialect (e.g.,
session.get_bind().dialect.name or session.bind.dialect.name) and only append
.with_for_update() when the dialect is not "sqlite" (for SQLite run the query
without .with_for_update(), or apply a SQLite-compatible lock if desired).
Ensure the logic still computes max_turn_number the same way and references
UserTurn.turn_number and conversation_id/normalized_id the same as before.
🧹 Nitpick comments (2)
tests/unit/app/endpoints/test_conversations.py (1)
869-900:test_sqlalchemy_error_in_get_conversation— test only verifies exception passthrough, not actual DB error handling.This test patches
validate_and_retrieve_conversationto directly raiseHTTPExceptionand then asserts the same exception comes out. It's effectively testing that FastAPI propagates anHTTPException— not that the endpoint orvalidate_and_retrieve_conversationcorrectly handles aSQLAlchemyError. The existingtest_sqlalchemy_error_in_retrieve_conversation(lines 970–1011) already covers the real DB error path. Consider whether this test adds meaningful coverage or is redundant.tests/unit/utils/test_query.py (1)
527-565: Test mock routing is well-structured, but consider verifying theUserTurnwas added.The
query_side_effectdispatch pattern is clean. However,mock_session.add.assert_called()only verifiesaddwas called at least once — it doesn't confirm aUserTurnobject was persisted. Since turn tracking is the core new behavior, a targeted assertion would strengthen coverage.Optional: verify UserTurn was added
mock_session.add.assert_called() mock_session.commit.assert_called_once() + + # Verify a UserTurn was added + from models.database.conversations import UserTurn + add_calls = mock_session.add.call_args_list + turn_adds = [c for c in add_calls if isinstance(c[0][0], UserTurn)] + assert len(turn_adds) == 1 + added_turn = turn_adds[0][0][0] + assert added_turn.turn_number == 1 + assert added_turn.provider == "provider1" + assert added_turn.model == "model1"
asimurka
force-pushed
the
rebase_conversation_history
branch
from
5c354fe to
43d06ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/utils/conversations.py`:
- Around line 400-415: The calculation for legacy_turns_count can go negative
when len(turns_metadata) > total_turns; change the logic around total_turns,
legacy_turns_count and the selection of turn_metadata (used in the loop over
turn_items_list and functions like _create_dummy_turn_metadata) to defend
against that: clamp legacy_turns_count to at least 0 (e.g., legacy_turns_count =
max(0, total_turns - len(turns_metadata))) or explicitly check if
len(turns_metadata) > total_turns and adjust metadata indexing, and add a
warning log when metadata outnumbers turns so orphaned metadata is visible
instead of silently skipped. Ensure subsequent indexing of turns_metadata uses
the adjusted metadata_index calculation to avoid negative offsets.
🧹 Nitpick comments (6)
src/utils/query.py (3)
547-566: Good:with_for_update()removed, resolving SQLite compatibility.The previous review flagged that
with_for_update()would fail on SQLite. This has been addressed by removing it entirely.However, without any locking, two concurrent requests for the same
conversation_idcan read the samemax_turn_numberand attempt to insert duplicate(conversation_id, turn_number)composite PKs. This will raise anIntegrityError(a subclass ofSQLAlchemyError), which is caught by the caller instore_query_results(line 406) and converted to an HTTP 500. For low-concurrency workloads this is acceptable, but for high-concurrency scenarios on the same conversation, consider a retry-on-conflict or a DB-level sequence/auto-increment forturn_number.
334-342: Parameter naming:modelshadows the Python built-in.The parameter
model: stron line 334 shadows Python's built-inmodel— actuallymodelisn't a Python built-in, so this is fine from a shadowing perspective. However, note the asymmetry: the public API usesmodel(combined "provider/model" string) while internally it's split intomodel_idandprovider_id. The docstring on line 354 could be clearer thatmodelis expected in"provider/model"format.
488-507: Docstring: document the expected ISO format forstarted_at/completed_at.The docstring says "timestamp when the conversation started/completed" but doesn't specify the expected format. Since the function calls
datetime.fromisoformat()internally (lines 556–557), invalid formats will raiseValueErrorthat isn't caught here (it would propagate as an unhandled exception, not as anSQLAlchemyError). Consider either documenting the expected ISO format or catchingValueErrorand wrapping it.tests/unit/utils/test_query.py (2)
529-565: Consider strengthening assertions intest_create_new_conversation.Line 564 uses
mock_session.add.assert_called()which only verifiesaddwas invoked at least once. The newtest_create_new_conversation_with_existing_turnstest (line 654–669) demonstrates a stronger pattern — inspectingcall_args_listto verify bothUserConversationandUserTurnobjects were added. Consider applying the same pattern here for consistency and to catch regressions where one of the twosession.add()calls might be accidentally removed.
543-549: Repeated mock routing logic could be extracted to a helper or fixture.The
query_side_effectfunction is duplicated across three tests with only the mock objects varying. A shared helper or parametrized fixture would reduce boilerplate.Also applies to: 589-594, 632-637
src/utils/conversations.py (1)
321-323: Messages are parsed twice: once during grouping, once during per-turn processing.
_parse_message_itemis called here solely to checkmessage.type == "user", but the resultingMessageobject is discarded. The same item is parsed again inside_process_turn_items(line 367). You can avoid the redundant construction by checking the role directly on the cast item:♻️ Suggested simplification
if item_type == "message": message_item = cast(MessageOutput, item) - message = _parse_message_item(message_item) - - if message.type == "user": + if message_item.role == "user":
asimurka
force-pushed
the
rebase_conversation_history
branch
from
43d06ab to
23a433d
Compare
jrobertboos
left a comment
jrobertboos
left a comment
There was a problem hiding this comment.
Overall LGTM. This PR though should be tied to a JIRA issue though :)
asimurka
changed the title
Done ;) |
4 participants
Description
This PR returns incorrectly rebased changes for conversation history enrichment.
It also fixes hidden bug for legacy conversations that have no metadata stored for older turns. Now the code correctly adds dummy metadata for oldest turns and real metadata for the newest turns.
Finally, locking of rows when turn is being persisted was removed because 1) no race conditions are expected here in practice, 2) the previous syntax did not work correctly with sqlite.
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Related Issue # LCORE-1078
Closes # LCORE-1166
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests