fix(gateway): honest recovery message for session-persistence failures instead of 'unknown error'
Two defects in _normalize_empty_agent_response surfaced together during a state.db lock-contention incident on an enterprise Slack deployment: - the error lookup used dict.get's default, which an explicit 'error': None value bypasses, rendering 'The request failed: None' / 'unknown error'; - persistence failures fell through to the generic branch, whose 'use /reset' advice is harmful for this failure mode (destroys conversation context, fixes nothing). Persistence-failed turns (failure_reason session_persistence_failed:*, with a legacy fallback on the error text) now get a dedicated message: storage was temporarily unavailable, the message was recorded, send it again — with a disk-specific variant. No /reset suggestion. All other branches unchanged.
This commit is contained in:
parent
2a9f5b3476
commit
01bc8a8752
|
|
@ -3487,8 +3487,33 @@ def _normalize_empty_agent_response(
|
|||
return response
|
||||
|
||||
if agent_result.get("failed"):
|
||||
error_detail = agent_result.get("error", "unknown error")
|
||||
# None-safe: the gateway result dict is built with
|
||||
# ``'error': holder.get('error')`` and can carry an EXPLICIT None,
|
||||
# which bypasses dict.get's default and would render
|
||||
# "The request failed: None".
|
||||
error_detail = agent_result.get("error") or "unknown error"
|
||||
error_str = str(error_detail).lower()
|
||||
# Session-persistence failures get a dedicated recovery message.
|
||||
# Suggesting /reset here would be actively harmful: it destroys the
|
||||
# user's conversation context and does nothing to fix the underlying
|
||||
# storage problem (lock contention, disk exhaustion, ...).
|
||||
failure_reason = str(agent_result.get("failure_reason") or "")
|
||||
if failure_reason.startswith("session_persistence_failed") or (
|
||||
"session storage" in error_str
|
||||
):
|
||||
if failure_reason.endswith(":disk") or "disk" in error_str:
|
||||
return (
|
||||
"⚠️ Session storage was temporarily unavailable, so this "
|
||||
"turn was stopped to protect your conversation history. "
|
||||
"Please check available disk space, then send your "
|
||||
"message again."
|
||||
)
|
||||
return (
|
||||
"⚠️ Session storage was temporarily unavailable, so this "
|
||||
"turn was stopped to protect your conversation history. "
|
||||
"Your message was recorded — please send it again in a "
|
||||
"moment."
|
||||
)
|
||||
is_context_failure = any(
|
||||
p in error_str
|
||||
for p in ("context", "token", "too large", "too long", "exceed", "payload")
|
||||
|
|
|
|||
|
|
@ -0,0 +1,128 @@
|
|||
"""Unit tests for persistence-failure-aware messaging in
|
||||
``_normalize_empty_agent_response``.
|
||||
|
||||
When a turn is stopped because session persistence failed (SQLite lock
|
||||
contention, disk exhaustion, ...), the user must NOT be told to /reset —
|
||||
that destroys their conversation context and does nothing to fix storage.
|
||||
They must also never see 'The request failed: None' when the gateway result
|
||||
dict carries an explicit ``error: None``.
|
||||
"""
|
||||
|
||||
import pytest
|
||||
|
||||
from gateway.run import _normalize_empty_agent_response
|
||||
|
||||
|
||||
class TestPersistenceFailureRecoveryMessage:
|
||||
"""Failed turns whose failure_reason marks a session-persistence
|
||||
failure get a dedicated recovery message: reassure the user their
|
||||
history is protected, tell them to resend — never suggest /reset."""
|
||||
|
||||
def test_locked_persistence_failure_gets_recovery_message(self):
|
||||
agent_result = {
|
||||
"final_response": "",
|
||||
"failed": True,
|
||||
"failure_reason": "session_persistence_failed:locked",
|
||||
"error": "session storage was locked by another writer",
|
||||
"api_calls": 2,
|
||||
}
|
||||
|
||||
response = _normalize_empty_agent_response(agent_result, "", history_len=10)
|
||||
|
||||
assert "send it again" in response.lower()
|
||||
assert "/reset" not in response
|
||||
assert "unknown error" not in response.lower()
|
||||
|
||||
def test_disk_persistence_failure_mentions_disk(self):
|
||||
agent_result = {
|
||||
"final_response": "",
|
||||
"failed": True,
|
||||
"failure_reason": "session_persistence_failed:disk",
|
||||
"error": "session storage write failed: disk full",
|
||||
"api_calls": 1,
|
||||
}
|
||||
|
||||
response = _normalize_empty_agent_response(agent_result, "", history_len=10)
|
||||
|
||||
assert "disk" in response.lower()
|
||||
assert "/reset" not in response
|
||||
assert "unknown error" not in response.lower()
|
||||
|
||||
def test_unknown_cause_persistence_failure_still_avoids_reset(self):
|
||||
agent_result = {
|
||||
"final_response": "",
|
||||
"failed": True,
|
||||
"failure_reason": "session_persistence_failed:unknown",
|
||||
"error": "session storage failure",
|
||||
"api_calls": 1,
|
||||
}
|
||||
|
||||
response = _normalize_empty_agent_response(agent_result, "", history_len=10)
|
||||
|
||||
assert "/reset" not in response
|
||||
assert "send it again" in response.lower()
|
||||
|
||||
def test_legacy_shape_error_text_mentioning_session_storage(self):
|
||||
"""Legacy failed results carry no failure_reason but an error text
|
||||
naming session storage — they must get the same recovery message."""
|
||||
agent_result = {
|
||||
"final_response": "",
|
||||
"failed": True,
|
||||
"error": "turn stopped: session storage unavailable",
|
||||
"api_calls": 1,
|
||||
}
|
||||
|
||||
response = _normalize_empty_agent_response(agent_result, "", history_len=10)
|
||||
|
||||
assert "/reset" not in response
|
||||
assert "send it again" in response.lower()
|
||||
|
||||
|
||||
class TestExplicitNoneErrorIsNoneSafe:
|
||||
"""The gateway result dict is built with ``'error': holder.get('error')``
|
||||
and can carry an EXPLICIT None, which bypasses dict.get defaults."""
|
||||
|
||||
def test_explicit_none_error_never_renders_none(self):
|
||||
agent_result = {
|
||||
"final_response": "",
|
||||
"failed": True,
|
||||
"error": None,
|
||||
"api_calls": 1,
|
||||
}
|
||||
|
||||
response = _normalize_empty_agent_response(agent_result, "", history_len=10)
|
||||
|
||||
assert "None" not in response
|
||||
# Non-persistence generic failures may legitimately say
|
||||
# 'unknown error' — the defect is rendering the literal None.
|
||||
assert "unknown error" in response.lower()
|
||||
|
||||
|
||||
class TestGenericFailureRegression:
|
||||
"""Non-persistence failures keep the existing byte-identical message."""
|
||||
|
||||
def test_provider_error_still_formats_request_failed(self):
|
||||
agent_result = {
|
||||
"final_response": "",
|
||||
"failed": True,
|
||||
"error": "provider exploded",
|
||||
"api_calls": 1,
|
||||
}
|
||||
|
||||
response = _normalize_empty_agent_response(agent_result, "", history_len=10)
|
||||
|
||||
assert "The request failed: provider exploded" in response
|
||||
assert "/reset" in response
|
||||
|
||||
def test_context_failure_branch_unchanged(self):
|
||||
agent_result = {
|
||||
"final_response": "",
|
||||
"failed": True,
|
||||
"error": "prompt exceeds context window",
|
||||
"api_calls": 1,
|
||||
}
|
||||
|
||||
response = _normalize_empty_agent_response(agent_result, "", history_len=60)
|
||||
|
||||
assert "context window" in response
|
||||
assert "/compact" in response
|
||||
Loading…
Reference in New Issue