Skip to content

Commit 665f73b

Browse files
jmelahmanclaude
andcommitted
feat(chat): delete never-used chat sessions when a send fails during setup
The send flow commits the user message partway through setup; a failure before that commit (LLM misconfig, cost limit, permission or hook rejection) leaves the pre-created session with only its SYSTEM root — no user-visible content. Best-effort hard-delete such sessions from the failure paths of _stream_chat_turn, guarded by a no-non-SYSTEM-messages check so a failure after the user message landed never touches a real conversation, and never masking the original error. This prevents most "unnamed husk" sidebar rows at the source; sends that never reach the backend at all are left to the background failed-chat cleanup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 4a8ed76 commit 665f73b

3 files changed

Lines changed: 243 additions & 0 deletions

File tree

backend/onyx/chat/process_message.py

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@
8181
from onyx.context.search.models import BaseFilters, SearchDoc
8282
from onyx.db.chat import (
8383
create_new_chat_message,
84+
delete_chat_session_if_never_used,
8485
get_chat_session_by_id,
8586
get_or_create_root_message,
8687
reserve_message_id,
@@ -1633,6 +1634,30 @@ def _read_stream() -> AnswerStream:
16331634
return _read_stream()
16341635

16351636

1637+
def _cleanup_failed_chat_session(chat_session_id: UUID | None) -> None:
1638+
"""Best-effort: hard-delete the session a failed send left with no
1639+
user-visible content, so it doesn't linger as an unnamed husk until the
1640+
background failed-chat cleanup reclaims it. A failure after the user
1641+
message was committed is a real conversation and is left untouched.
1642+
Never raises — the original error must propagate unmasked.
1643+
"""
1644+
if chat_session_id is None:
1645+
return
1646+
try:
1647+
with get_session_with_current_tenant() as db_session:
1648+
if delete_chat_session_if_never_used(
1649+
chat_session_id=chat_session_id, db_session=db_session
1650+
):
1651+
logger.info(
1652+
"Deleted never-used chat session %s after failed send",
1653+
chat_session_id,
1654+
)
1655+
except Exception:
1656+
logger.exception(
1657+
"Cleanup of never-used chat session %s failed", chat_session_id
1658+
)
1659+
1660+
16361661
def _stream_chat_turn(
16371662
new_msg_req: SendMessageRequest,
16381663
user: User,
@@ -1688,6 +1713,19 @@ def _stream_chat_turn(
16881713
pre_run_packets: list[AnswerStreamPart] = []
16891714
run_started = False
16901715

1716+
def _failed_chat_session_id() -> UUID | None:
1717+
"""The session this failed send targeted: known from setup once built,
1718+
else from the request, else from the CreateChatSessionID packet when
1719+
the send created the session itself."""
1720+
if setup is not None:
1721+
return setup.chat_session.id
1722+
if new_msg_req.chat_session_id:
1723+
return new_msg_req.chat_session_id
1724+
for packet in pre_run_packets:
1725+
if isinstance(packet, CreateChatSessionID):
1726+
return packet.chat_session_id
1727+
return None
1728+
16911729
try:
16921730
with get_session_with_current_tenant() as setup_db_session:
16931731
try:
@@ -1791,6 +1829,7 @@ def _stream_chat_turn(
17911829
except OnyxError as e:
17921830
if e.error_code is not OnyxErrorCode.QUERY_REJECTED:
17931831
log_onyx_error(e)
1832+
_cleanup_failed_chat_session(_failed_chat_session_id())
17941833
yield StreamingError(
17951834
error=e.detail,
17961835
error_code=e.error_code.code,
@@ -1800,6 +1839,7 @@ def _stream_chat_turn(
18001839

18011840
except ValueError as e:
18021841
logger.exception("Failed to process chat message.")
1842+
_cleanup_failed_chat_session(_failed_chat_session_id())
18031843
yield StreamingError(
18041844
error=str(e),
18051845
error_code="VALIDATION_ERROR",
@@ -1833,6 +1873,10 @@ def _stream_chat_turn(
18331873
except Exception as e:
18341874
logger.exception("Failed to process chat message due to %s", e)
18351875
stack_trace = traceback.format_exc()
1876+
# No-ops when the user message was already committed (the LLM only runs
1877+
# after it), so this only fires for setup failures. Same applies to the
1878+
# handlers above; EmptyLLMResponseError is post-commit by construction.
1879+
_cleanup_failed_chat_session(_failed_chat_session_id())
18361880

18371881
llm = setup.llms[0] if setup else None
18381882
if llm:

backend/onyx/db/chat.py

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -450,6 +450,42 @@ def get_chat_sessions_older_than(
450450
return returned_sessions
451451

452452

453+
def delete_chat_session_if_never_used(
454+
chat_session_id: UUID,
455+
db_session: Session,
456+
) -> bool:
457+
"""Hard-delete a chat session iff it has never contained a non-SYSTEM
458+
message, i.e. holds no user-visible content.
459+
460+
Used best-effort by the send-message failure path: a failure before the
461+
user message was committed would otherwise leave an unnamed husk session
462+
behind. Returns True if the session was deleted.
463+
"""
464+
session_exists = db_session.execute(
465+
select(ChatSession.id).where(ChatSession.id == chat_session_id)
466+
).scalar_one_or_none()
467+
if session_exists is None:
468+
return False
469+
470+
has_user_visible_message = db_session.execute(
471+
select(ChatMessage.id)
472+
.where(ChatMessage.chat_session_id == chat_session_id)
473+
.where(ChatMessage.message_type != MessageType.SYSTEM)
474+
.limit(1)
475+
).scalar_one_or_none()
476+
if has_user_visible_message is not None:
477+
return False
478+
479+
delete_chat_session(
480+
user_id=None,
481+
chat_session_id=chat_session_id,
482+
db_session=db_session,
483+
include_deleted=True,
484+
hard_delete=True,
485+
)
486+
return True
487+
488+
453489
def get_chat_message(
454490
chat_message_id: int,
455491
user_id: UUID | None,
Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,163 @@
1+
"""Tests for husk prevention in the send-message failure path.
2+
3+
A send that fails before the user message is committed leaves a session with
4+
no user-visible content; the failure path is expected to delete it so it never
5+
shows up as an unnamed husk in the sidebar. A failure after the user message
6+
landed is a real conversation and must survive.
7+
"""
8+
9+
import uuid
10+
from collections.abc import Generator
11+
from unittest.mock import patch
12+
from uuid import UUID
13+
14+
import pytest
15+
from sqlalchemy import select
16+
from sqlalchemy.orm import Session
17+
18+
from onyx.chat.models import AnswerStreamPart, StreamingError
19+
from onyx.chat.process_message import handle_stream_message_objects
20+
from onyx.configs.constants import MessageType
21+
from onyx.db.chat import create_chat_session, delete_chat_session
22+
from onyx.db.models import ChatMessage, ChatSession, Persona, User
23+
from onyx.db.persona import upsert_persona
24+
from onyx.server.query_and_chat.models import SendMessageRequest
25+
from tests.external_dependency_unit.answer.conftest import ensure_default_llm_provider
26+
from tests.external_dependency_unit.conftest import create_test_user
27+
28+
29+
@pytest.fixture
30+
def test_user(db_session: Session) -> User:
31+
return create_test_user(db_session, email_prefix="failed_send_cleanup")
32+
33+
34+
@pytest.fixture
35+
def test_persona(db_session: Session) -> Persona:
36+
ensure_default_llm_provider(db_session)
37+
return upsert_persona(
38+
user=None,
39+
name=f"Failed Send Cleanup Persona {uuid.uuid4()}",
40+
description="Tool-less persona for send-failure tests",
41+
starter_messages=None,
42+
system_prompt=None,
43+
task_prompt=None,
44+
datetime_aware=None,
45+
is_public=True,
46+
db_session=db_session,
47+
tool_ids=[],
48+
document_set_ids=None,
49+
is_listed=True,
50+
default_model_configuration_id=None,
51+
)
52+
53+
54+
def _session_row_exists(db_session: Session, chat_session_id: UUID) -> bool:
55+
# Plain SELECT on purpose: Session.get() on an identity-map instance whose
56+
# row was deleted by another session raises ObjectDeletedError on refresh.
57+
return (
58+
db_session.execute(
59+
select(ChatSession.id).where(ChatSession.id == chat_session_id)
60+
).first()
61+
is not None
62+
)
63+
64+
65+
@pytest.fixture
66+
def session_tracker(db_session: Session) -> Generator[list[UUID], None, None]:
67+
created: list[UUID] = []
68+
yield created
69+
db_session.expunge_all()
70+
for session_id in created:
71+
if _session_row_exists(db_session, session_id):
72+
delete_chat_session(
73+
user_id=None,
74+
chat_session_id=session_id,
75+
db_session=db_session,
76+
include_deleted=True,
77+
hard_delete=True,
78+
)
79+
80+
81+
def _consume_stream_expecting_error(
82+
new_msg_req: SendMessageRequest, user: User
83+
) -> None:
84+
packets: list[AnswerStreamPart] = list(
85+
handle_stream_message_objects(new_msg_req=new_msg_req, user=user)
86+
)
87+
assert any(isinstance(packet, StreamingError) for packet in packets)
88+
89+
90+
def test_failure_before_user_message_commit_deletes_session(
91+
db_session: Session,
92+
test_user: User,
93+
test_persona: Persona,
94+
session_tracker: list[UUID],
95+
) -> None:
96+
chat_session = create_chat_session(
97+
db_session=db_session,
98+
description=None,
99+
user_id=test_user.id,
100+
persona_id=test_persona.id,
101+
)
102+
session_tracker.append(chat_session.id)
103+
# Release this session's snapshot so we observe the flow's own commits.
104+
db_session.commit()
105+
106+
# verify_user_files runs during setup, before the user message is written.
107+
with patch(
108+
"onyx.chat.process_message.verify_user_files",
109+
side_effect=RuntimeError("forced setup failure"),
110+
):
111+
_consume_stream_expecting_error(
112+
SendMessageRequest(message="hello", chat_session_id=chat_session.id),
113+
test_user,
114+
)
115+
116+
db_session.expunge_all()
117+
assert not _session_row_exists(db_session, chat_session.id)
118+
assert (
119+
db_session.execute(
120+
select(ChatMessage.id).where(ChatMessage.chat_session_id == chat_session.id)
121+
).first()
122+
is None
123+
)
124+
125+
126+
def test_failure_after_user_message_commit_keeps_session(
127+
db_session: Session,
128+
test_user: User,
129+
test_persona: Persona,
130+
session_tracker: list[UUID],
131+
) -> None:
132+
chat_session = create_chat_session(
133+
db_session=db_session,
134+
description=None,
135+
user_id=test_user.id,
136+
persona_id=test_persona.id,
137+
)
138+
session_tracker.append(chat_session.id)
139+
db_session.commit()
140+
141+
# reserve_message_id runs right after the user message is committed.
142+
with patch(
143+
"onyx.chat.process_message.reserve_message_id",
144+
side_effect=RuntimeError("forced post-commit failure"),
145+
):
146+
_consume_stream_expecting_error(
147+
SendMessageRequest(message="hello", chat_session_id=chat_session.id),
148+
test_user,
149+
)
150+
151+
db_session.expunge_all()
152+
assert _session_row_exists(db_session, chat_session.id)
153+
user_messages = (
154+
db_session.execute(
155+
select(ChatMessage)
156+
.where(ChatMessage.chat_session_id == chat_session.id)
157+
.where(ChatMessage.message_type == MessageType.USER)
158+
)
159+
.scalars()
160+
.all()
161+
)
162+
assert len(user_messages) == 1
163+
assert user_messages[0].message == "hello"

0 commit comments

Comments
 (0)