Properly delete conversation #476
Conversation
|
Warning Rate limit exceeded@omertuc has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughAdds a local conversation deletion step to the conversations DELETE endpoint. After remote deletion via Llama Stack, the endpoint now calls a new utility function to remove the conversation from local storage. Tests are updated to patch the new utility to avoid side effects. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as Conversations API
participant Remote as Llama Stack
participant Utils as utils.endpoints
participant DB
Client->>API: DELETE /conversations/{id}
API->>Remote: Delete session {id}
Remote-->>API: 200 OK
Note over API: Remote deletion succeeded
API->>Utils: delete_conversation(id)
Utils->>DB: Find & delete UserConversation(id)
DB-->>Utils: Commit result
Utils-->>API: None
API-->>Client: 200 OK
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
See lightspeed-core/lightspeed-stack#476 lightspeed-stack is now also more strict with not accepting non-existent config, so we have to remove some old config we used to pass that doesn't exist anymore, otherwise lightspeed-stack would fail to start.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/utils/endpoints.py (1)
21-36: Return a boolean for idempotent feedbackReturning True/False (deleted vs. not found) can improve call-site logging/metrics without extra queries.
-def delete_conversation(conversation_id: str) -> None: +def delete_conversation(conversation_id: str) -> bool: @@ - if db_conversation: + if db_conversation: session.delete(db_conversation) session.commit() logger.info("Deleted conversation %s from local database", conversation_id) - else: + return True + else: logger.info( "Conversation %s not found in local database, it may have already been deleted", conversation_id, ) + return Falsetests/unit/app/endpoints/test_conversations.py (1)
566-568: Assert the local cleanup was invokedYou patch delete_conversation but don’t assert it was called. Add an assertion to ensure the endpoint performs local cleanup.
Apply within the selected lines:
- mocker.patch("app.endpoints.conversations.delete_conversation") + mock_delete = mocker.patch("app.endpoints.conversations.delete_conversation")Then add this assertion after the response is created:
# Assert local cleanup was called mock_delete.assert_called_once_with(conversation_id=VALID_CONVERSATION_ID)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/app/endpoints/conversations.py(2 hunks)src/utils/endpoints.py(1 hunks)tests/unit/app/endpoints/test_conversations.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/utils/endpoints.py (2)
src/app/database.py (1)
get_session(34-40)src/models/database/conversations.py (1)
UserConversation(11-36)
src/app/endpoints/conversations.py (1)
src/utils/endpoints.py (1)
delete_conversation(21-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
🔇 Additional comments (1)
src/app/endpoints/conversations.py (1)
23-27: LGTM: import of delete_conversationImport grouping looks correct and localized to endpoint usage.
See bug MGMT-21651 We saw that when deleting a conversation, it only gets deleted from the llama-stack database, but not from the lightspeed-stack conversations table. This commit makes it that the conversation is deleted from both places, ensuring it does not show up when the user lists conversations. Also fix a bug where the conversation ownership validation was not effective because its return value was ignored.
What do you mean? |
the user_id and conversation_id used to be a compound key in database. And because user_id can't be mocked (it's taken from the token) it means, that nobody else will be able to delete (or even read) user conversation, even if anybody tries to send bunch of random conversation_ids into the REST API. |
They're not a compound primary key |
does not it mean than anybody can access any conversation then? |
No, we use |
yup that's good. Also user_id is stored in db and conversation retrieved by: so it looked like user_id+conversation_id are compound key. Whatever it look ok then |
Ah I see. Yeah that's a bug, IMO this should only query by |
See lightspeed-core/lightspeed-stack#476 lightspeed-stack is now also more strict with not accepting non-existent config, so we have to remove some old config we used to pass that doesn't exist anymore, otherwise lightspeed-stack would fail to start.
Followup #484 |
2 participants
Description
We saw that when deleting a conversation, it only gets deleted from the llama-stack database, but not from the lightspeed-stack conversations table.
This commit makes it that the conversation is deleted from both places, ensuring it does not show up when the user lists conversations.
Also fix a bug where the conversation ownership validation was not
Type of change
Related Tickets & Documents
MGMT-21651
Checklist before requesting a review
Testing
Unit tests have been adjusted. I also tested manually and saw the conversation no longer appearing after deletion.
Summary by CodeRabbit
Bug Fixes
Tests