fix conversation delete endpoint #397
Conversation
Signed-off-by: Stephanie <[email protected]>
WalkthroughAdds guards for missing agent sessions in conversation GET/DELETE handlers and in get_agent; when no sessions exist the endpoints/logic now return HTTP 404 with structured detail and re-raise HTTPException. DELETE uses the first session_id only when a session exists. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Conversations API
participant SessionSvc as Session Service
Client->>API: GET/DELETE /conversations/{conversation_id}
API->>SessionSvc: list sessions for conversation_id
alt sessions found
API->>SessionSvc: (DELETE) delete using first session_id
API-->>Client: 200 OK (GET data or DELETE confirmation)
else no sessions
API-->>Client: 404 Conversation not found
end
sequenceDiagram
participant Caller
participant Utils as get_agent()
participant SessionSvc as Session Service
Caller->>Utils: get_agent(conversation_id?)
alt conversation_id provided
Utils->>SessionSvc: list sessions for conversation_id
alt sessions empty
Utils-->>Caller: 404 Conversation not found
else sessions present
Utils-->>Caller: session_id from first session
end
else no conversation_id
Utils-->>Caller: create new session (existing flow)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 (
|
![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 (1)
src/utils/endpoints.py (1)
112-120: Good defensive 404 on missing sessions; consider preflight check and narrower retrieval handlingThe guard fixes the IndexError. Two follow-ups:
- Preflight sessions before creating/initializing and then deleting an orphan agent to avoid unnecessary API churn in the “existing conversation” path.
- Treat NotFoundError on retrieve(agent_id=conversation_id) as “no existing agent” rather than bubbling up unexpectedly.
Example preflight restructure (outside this hunk):
# Before creating/initializing a new agent: with suppress(ValueError, NotFoundError): agent_response = await client.agents.retrieve(agent_id=conversation_id) existing_agent_id = agent_response.agent_id if existing_agent_id: sessions_response = await client.agents.session.list(agent_id=conversation_id) if not sessions_response or not sessions_response.data: logger.error("No sessions found for conversation %s", conversation_id) raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, detail={ "response": "Conversation not found", "cause": f"Conversation {conversation_id} could not be retrieved: no sessions found", }, ) # Only then create/initialize AsyncAgent and rewire _agent_id as you already doNitpick: the “session response: %s” log is quite verbose for INFO; consider DEBUG.
Note: The direct assignment to agent._agent_id continues to align with the known workaround for missing agent rehydration (see retrieved learnings).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/endpoints/conversations.py(4 hunks)src/utils/endpoints.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-06T06:02:21.060Z
Learnt from: eranco74
PR: lightspeed-core/lightspeed-stack#348
File: src/utils/endpoints.py:91-94
Timestamp: 2025-08-06T06:02:21.060Z
Learning: The direct assignment to `agent._agent_id` in `src/utils/endpoints.py` is a necessary workaround for the missing agent rehydration feature in the LLS client SDK. This allows preserving conversation IDs when handling existing agents.
Applied to files:
src/utils/endpoints.py
🔇 Additional comments (2)
src/app/endpoints/conversations.py (2)
295-296: Explicitly re-raising HTTPException is correctPrevents the broad Exception handler from masking expected HTTP responses. LGTM.
396-397: Explicit HTTPException re-raise is correctPrevents legitimate 4xx/5xx flows from being swallowed by the generic handler. LGTM.
| agent_sessions = (await client.agents.session.list(agent_id=agent_id)).data | ||
|
|
||
| if not agent_sessions: | ||
| logger.error("No sessions found for conversation %s", conversation_id) | ||
| raise HTTPException( | ||
| status_code=status.HTTP_404_NOT_FOUND, | ||
| detail={ | ||
| "response": "Conversation not found", | ||
| "cause": f"Conversation {conversation_id} could not be deleted: no sessions found", | ||
| }, | ||
| ) | ||
|
|
||
| session_id = str(agent_sessions[0].get("session_id")) | ||
|
|
||
| await client.agents.session.delete(agent_id=agent_id, session_id=session_id) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider deleting all sessions rather than just the first
If a conversation can have multiple sessions, deleting only the first may leave residual state. If the intended behavior is to “delete conversation,” consider deleting all sessions. If only one session is ever expected, add a brief comment stating that assumption.
Optional refactor within this block:
- agent_sessions = (await client.agents.session.list(agent_id=agent_id)).data
+ agent_sessions = (await client.agents.session.list(agent_id=agent_id)).data
if not agent_sessions:
logger.error("No sessions found for conversation %s", conversation_id)
raise HTTPException(
status_code=status.HTTP_404_NOT_FOUND,
detail={
"response": "Conversation not found",
"cause": f"Conversation {conversation_id} could not be deleted: no sessions found",
},
)
-
- session_id = str(agent_sessions[0].get("session_id"))
-
- await client.agents.session.delete(agent_id=agent_id, session_id=session_id)
+ # Delete all sessions to ensure full conversation cleanup
+ for sess in agent_sessions:
+ sid = sess.get("session_id")
+ if sid:
+ await client.agents.session.delete(agent_id=agent_id, session_id=str(sid))To confirm intended semantics with the upstream SDK, please verify whether deleting a single session constitutes a full “conversation delete,” or whether all sessions should be removed.
🌐 Web query:
In llama-stack (or llama_stack_client) APIs, does deleting an agent session via agents.session.delete remove only that session or the entire agent/conversation? Please provide the official docs reference for expected behavior.
💡 Result:
Deleting an agent session using agents.session.delete (or the equivalent endpoint) in Llama Stack APIs removes only that specific session—and all of its turns—not the entire agent or other conversations. This aligns with both the method signatures and descriptions found in the official source code and API documentation.
Official documentation and references:
-
In the Llama Stack agents API source code, the relevant method is:
@webmethod(route="/agents/{agent_id}/session/{session_id}", method="DELETE") async def delete_agents_session( self, session_id: str, agent_id: str, ) -> None: """Delete an agent session by its ID and its associated turns. :param session_id: The ID of the session to delete. :param agent_id: The ID of the agent to delete the session for. """
The docstring explicitly states: "Delete an agent session by its ID and its associated turns." This means only the specified session (the conversation thread) and any subcomponents such as turns/messages in that session are deleted, not the agent or its full set of sessions5.
-
Conversely, there is a separate endpoint for deleting an entire agent:
@webmethod(route="/agents/{agent_id}", method="DELETE") async def delete_agent( self, agent_id: str, ) -> None: """Delete an agent by its ID and its associated sessions and turns. :param agent_id: The ID of the agent to delete. """
This method deletes the entire agent and all its associated sessions/conversations5.
-
The official Llama Stack documentation3 further clarifies that an agent can have multiple sessions (each representing a separate conversation), and deleting a session affects only that session, not the agent or other sessions1.
Summary Table:
| Operation | Deletes Only Current Session? | Deletes Entire Agent/All Sessions? | Official Source |
|---|---|---|---|
agents.session.delete |
Yes | No | 5 |
agents.delete |
No | Yes | 5 |
Therefore, per official Llama Stack references, deleting a session removes only that session and its turns, leaving the agent (and its other sessions) intact.
Reference: GitHub source code and API docstrings, Official agent documentation3.
Citations:
Citations:
- 1: https://llama-stack.readthedocs.io/en/latest/building_applications/agent.html
- 2: https://docs.aws.amazon.com/bedrock/latest/userguide/sessions-delete-session.html
- 3: https://llama-stack.readthedocs.io/en/latest/building_applications/responses_vs_agents.html
- 4: https://rocm.docs.amd.com/projects/ai-developer-hub/en/latest/notebooks/inference/llama-stack-rocm.html
- 5: https://github.com/meta-llama/llama-stack/blob/main/llama_stack/apis/agents/agents.py
- 6: Improve DELETE response consistency by returning HTTP 204 No Content instead of null llamastack/llama-stack#3090
- 7: https://www.youtube.com/watch?v=8b_N660NlOE
- 8: https://llama-stack.readthedocs.io/en/latest/distributions/building_distro.html
- 9: https://git.kvant.cloud/phoenix-oss/llama-stack/commits/commit/314ee09ae326f0d3f9e0fbfc07c8dac8e1b4fce3?page=2
- 10: https://developers.redhat.com/articles/2025/04/02/practical-guide-llama-stack-nodejs-developers
Action: Delete all agent sessions (or document a single-session assumption) — agents.session.delete removes only a single session
Verified: Llama Stack docs/source confirm agents.session.delete removes only the specified session and its turns; deleting an entire agent (and all sessions) is a separate endpoint (see agents.delete in the upstream repo).
- File to update:
- src/app/endpoints/conversations.py (lines ~354-369)
Recommended change (replace deleting only the first session with deleting all sessions; handle missing IDs and log failures):
- session_id = str(agent_sessions[0].get("session_id"))
- await client.agents.session.delete(agent_id=agent_id, session_id=session_id)
+ # Delete all sessions to ensure full conversation cleanup
+ for sess in agent_sessions:
+ sid = sess.get("session_id")
+ if not sid:
+ continue
+ try:
+ await client.agents.session.delete(agent_id=agent_id, session_id=str(sid))
+ except Exception as e:
+ logger.warning("Failed to delete session %s for agent %s: %s", sid, agent_id, e)Alternative: if the intent is to remove the entire agent (and all sessions) use the agent delete endpoint (e.g., client.agents.delete(agent_id=agent_id)) per upstream docs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| agent_sessions = (await client.agents.session.list(agent_id=agent_id)).data | |
| if not agent_sessions: | |
| logger.error("No sessions found for conversation %s", conversation_id) | |
| raise HTTPException( | |
| status_code=status.HTTP_404_NOT_FOUND, | |
| detail={ | |
| "response": "Conversation not found", | |
| "cause": f"Conversation {conversation_id} could not be deleted: no sessions found", | |
| }, | |
| ) | |
| session_id = str(agent_sessions[0].get("session_id")) | |
| await client.agents.session.delete(agent_id=agent_id, session_id=session_id) | |
| agent_sessions = (await client.agents.session.list(agent_id=agent_id)).data | |
| if not agent_sessions: | |
| logger.error("No sessions found for conversation %s", conversation_id) | |
| raise HTTPException( | |
| status_code=status.HTTP_404_NOT_FOUND, | |
| detail={ | |
| "response": "Conversation not found", | |
| "cause": f"Conversation {conversation_id} could not be deleted: no sessions found", | |
| }, | |
| ) | |
| # Delete all sessions to ensure full conversation cleanup | |
| for sess in agent_sessions: | |
| sid = sess.get("session_id") | |
| if not sid: | |
| continue | |
| try: | |
| await client.agents.session.delete(agent_id=agent_id, session_id=str(sid)) | |
| except Exception as e: | |
| logger.warning("Failed to delete session %s for agent %s: %s", sid, agent_id, e) |
🤖 Prompt for AI Agents
In src/app/endpoints/conversations.py around lines 354 to 369, the code only
deletes the first session returned (agents.session.delete removes a single
session); change it to iterate over all returned agent_sessions and call
client.agents.session.delete for each session_id (safely extract session_id with
a fallback check, skip or log and continue if a session_id is missing), wrap
each delete call in try/except to log failures per-session and continue, and if
the intended behavior is to remove the entire agent instead, replace the
per-session deletes with a single client.agents.delete(agent_id=agent_id) call
and log that choice.
umago
left a comment
umago
left a comment
There was a problem hiding this comment.
Thanks for the patch @yangcao77. I think we should keep idempotency for DELETE as a REST API design, it's a better user experience IMHO.
Let me know what you think
| "response": "Conversation not found", | ||
| "cause": f"Conversation {conversation_id} could not be deleted: no sessions found", | ||
| }, | ||
| ) |
There was a problem hiding this comment.
I don't think we should be returning 404 on DELETE, for REST APIs I think the approach needs to be idempotent.
If the user asks "delete this" and the end state (resource gone) is already true, the operation is logically successful.
src/utils/endpoints.py
Outdated
| "cause": f"Conversation {conversation_id} could not be retrieved.", | ||
| }, | ||
| ) | ||
| session_id = str(sessions_response.data[0]["session_id"]) |
There was a problem hiding this comment.
most a nit here but, EAFP is more "pythonic" than LBYL here:
try:
session_id = str(sessions_response.data[0]["session_id"])
except IndexError:
log.error(...
…to fix-delete-conversation
Signed-off-by: Stephanie <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
src/app/endpoints/conversations.py (3)
260-266: Avoid using 'None' as session_id; safely extract a valid session_id or 404If the first session entry lacks a
session_id,str(None)becomes "None" and the subsequent retrieve will fail in a confusing way. Extract a valid non-empty session_id or return 404.Apply:
- session_id = str(agent_sessions[0].get("session_id")) + # Safely extract a valid session_id from returned sessions + session_id = next( + (str(s["session_id"]) for s in agent_sessions if s.get("session_id")), + None, + ) + if not session_id: + logger.error("No valid session_id found for conversation %s", conversation_id) + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail={ + "response": "Conversation not found", + "cause": f"Conversation {conversation_id} could not be retrieved: missing session_id.", + }, + )
56-61: OpenAPI response example uses 'message' but the model returns 'response'The example schema for 200 in
conversation_delete_responsesusesmessage, whereasConversationDeleteResponsedefinesresponse. This misleads generated client SDKs and API consumers.Apply:
- "success": True, - "message": "Conversation deleted successfully", + "success": True, + "response": "Conversation deleted successfully",
386-394: Treat NotFound during per-session delete as success to keep DELETE idempotentDELETE currently returns 404 when the Llama Stack client raises NotFoundError during session.delete. Idempotent semantics are preferable here — if the session is already gone, return a successful ConversationDeleteResponse instead of 404.
Files/locations to change
- src/app/endpoints/conversations.py — replace the except NotFoundError block in delete_conversation_endpoint_handler (around lines ~386–394).
- tests/unit/app/endpoints/test_conversations.py — update the delete test
test_llama_stack_not_found_error(it currently asserts a 404) to expect a successful ConversationDeleteResponse.Suggested code change (replace NotFoundError handler):
- except NotFoundError as e: - logger.error("Conversation not found: %s", e) - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail={ - "response": "Conversation not found", - "cause": f"Conversation {conversation_id} could not be deleted: {str(e)}", - }, - ) from e + except NotFoundError as e: + logger.info("Session already absent when deleting conversation %s: %s", conversation_id, e) + return ConversationDeleteResponse( + conversation_id=conversation_id, + success=True, + response="Conversation deleted successfully", + )Tests to update
- Change tests/unit/app/endpoints/test_conversations.py::test_llama_stack_not_found_error to assert a ConversationDeleteResponse with success True and response "Conversation deleted successfully" instead of expecting an HTTPException/404.
🧹 Nitpick comments (3)
tests/unit/app/endpoints/test_conversations.py (2)
507-513: Also assert that session.list is called with the correct agent_idStrengthen the test by verifying the list call (precondition to delete) is made with the expected arguments.
You can add this assertion after the existing ones:
mock_client.agents.session.list.assert_called_once_with( agent_id=VALID_CONVERSATION_ID )
485-506: Add coverage for “no sessions” -> idempotent success pathCurrently there’s no test for the case where
agents.session.list(...).datais empty. Add a test to ensure the endpoint returns success without invokingdelete.Proposed test:
@pytest.mark.asyncio async def test_conversation_deletion_when_no_sessions_returns_success(mocker, setup_configuration): mocker.patch("app.endpoints.conversations.configuration", setup_configuration) mocker.patch("app.endpoints.conversations.check_suid", return_value=True) mocker.patch("app.endpoints.conversations.validate_conversation_ownership") mock_client = mocker.AsyncMock() mock_client.agents.session.list.return_value = mocker.Mock(data=[]) mock_client_holder = mocker.patch("app.endpoints.conversations.AsyncLlamaStackClientHolder") mock_client_holder.return_value.get_client.return_value = mock_client response = await delete_conversation_endpoint_handler(VALID_CONVERSATION_ID, auth=MOCK_AUTH) assert isinstance(response, ConversationDeleteResponse) assert response.conversation_id == VALID_CONVERSATION_ID assert response.success is True assert response.response == "Conversation deleted successfully" mock_client.agents.session.delete.assert_not_called()Would you like me to add a follow-up test for the multiple-sessions scenario as well?
src/app/endpoints/conversations.py (1)
365-367: Deleting only the first session may leave residual state; consider deleting all sessionsIf an agent can have multiple sessions, deleting just the first leaves others intact. Either:
- delete all returned sessions, or
- document that an agent has at most one session.
Replace the single delete with a loop:
- session_id = str(agent_sessions[0].get("session_id")) - - await client.agents.session.delete(agent_id=agent_id, session_id=session_id) + # Delete all sessions to ensure full conversation cleanup + for sess in agent_sessions: + sid = sess.get("session_id") + if not sid: + logger.warning("Skipping session without session_id for agent %s: %s", agent_id, sess) + continue + try: + await client.agents.session.delete(agent_id=agent_id, session_id=str(sid)) + except Exception as e: + # Log and continue to keep the operation best-effort idempotent + logger.warning("Failed to delete session %s for agent %s: %s", sid, agent_id, e)If the product intent is to remove the entire “conversation” entity (agent and all sessions), consider calling
client.agents.delete(agent_id=agent_id)instead; otherwise, please add a short comment documenting the chosen semantics.
📜 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 settings in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/app/endpoints/conversations.py(4 hunks)src/utils/endpoints.py(1 hunks)tests/unit/app/endpoints/test_conversations.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/endpoints.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/app/endpoints/conversations.py (1)
src/models/responses.py (1)
ConversationDeleteResponse(401-434)
⏰ 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 (4)
tests/unit/app/endpoints/test_conversations.py (1)
493-496: Good setup to exercise the deletion pathMocking
agents.session.listto return an existing session makes the test meaningful and aligned with the new endpoint flow.src/app/endpoints/conversations.py (3)
251-259: Good 404 guard for conversations with no sessionsReturning 404 when no sessions are found for a valid ID is appropriate for GET and fixes the earlier IndexError path.
295-296: Preserving HTTPException semantics is correctRe-raising
HTTPExceptionprevents double-wrapping and keeps intended status codes.
354-364: Idempotent DELETE behavior when no sessions exist looks goodReturning success when no sessions are found matches idempotent DELETE semantics and avoids unnecessary 404s.
|
@umago thanks for the review. I have pushed a new commit to address your review comments. PTAL with you have time. |
umago
left a comment
umago
left a comment
There was a problem hiding this comment.
Thank you @yangcao77 it LGTM now
4 participants
Description
This PR fixes the
DELETE /conversations/{conversation_id}endpointbefore this change:
with this change:
This PR also fixes the index out of range error when passing in a deleted
conversation_idto query or get conversation requests.before this change:
after this change:
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit