refactor(gateway): share error-text blob between not_found classifiers
Follow-up to the #55780 dead-target not_found blast-radius fix (merged in #56225). classify_send_error and is_chat_level_not_found each built their own lowercased error blob, but divergently: classify_send_error appended the exception CLASS NAME while is_chat_level_not_found did not. A caller passing exc= to both could get inconsistent answers on the same failure. - Extract _error_blob(exc, error_text) as the single source of truth both classifiers use (str(exc) when non-empty + class name; no stray leading space). - Align is_chat_level_not_found's signature to (exc, error_text), matching classify_send_error, removing the swapped-positional footgun; update the sole caller and the three tests to keyword form. - Add a regression guard asserting _error_blob keeps the class name. Surfaced by the hermes-pr-review Phase 2c structured review of #56225.
This commit is contained in:
parent
01e681aa48
commit
53b017f03e
3 changed files with 47 additions and 18 deletions
|
|
@ -133,7 +133,7 @@ def _classify_dead_from_error_text(error_text: Optional[str]) -> Optional[str]:
|
|||
# Only a whole-chat not_found means the target is dead — a deleted forum topic
|
||||
# or an edited-away message must not mark the entire chat (and all of its future
|
||||
# deliveries) dead. See gateway.dead_targets' documented scope.
|
||||
if kind == "not_found" and not is_chat_level_not_found(error_text):
|
||||
if kind == "not_found" and not is_chat_level_not_found(error_text=error_text):
|
||||
return None
|
||||
return kind
|
||||
|
||||
|
|
|
|||
|
|
@ -1931,6 +1931,26 @@ _SUBCHAT_NOT_FOUND_SUBSTRINGS = (
|
|||
)
|
||||
|
||||
|
||||
def _error_blob(exc: Optional[BaseException] = None, error_text: str = "") -> str:
|
||||
"""Build the lowercased text blob both send-error classifiers match against.
|
||||
|
||||
Single source of truth so ``classify_send_error`` and
|
||||
``is_chat_level_not_found`` can never drift (e.g. one including the
|
||||
exception class name and the other not) and silently disagree on the same
|
||||
failure. Includes ``str(exc)`` (when non-empty) and the exception's class
|
||||
name, plus any explicit ``error_text``.
|
||||
"""
|
||||
parts = []
|
||||
if error_text:
|
||||
parts.append(error_text)
|
||||
if exc is not None:
|
||||
exc_str = str(exc)
|
||||
if exc_str:
|
||||
parts.append(exc_str)
|
||||
parts.append(exc.__class__.__name__)
|
||||
return " ".join(parts).lower()
|
||||
|
||||
|
||||
def classify_send_error(exc: Optional[BaseException], error_text: str = "") -> str:
|
||||
"""Map a send exception / error string to a :data:`SEND_ERROR_KINDS` value.
|
||||
|
||||
|
|
@ -1939,13 +1959,7 @@ def classify_send_error(exc: Optional[BaseException], error_text: str = "") -> s
|
|||
use. Conservative — anything unrecognized returns ``"unknown"`` so callers
|
||||
never mistake an unclassified failure for a benign one.
|
||||
"""
|
||||
parts = []
|
||||
if error_text:
|
||||
parts.append(error_text)
|
||||
if exc is not None:
|
||||
parts.append(str(exc))
|
||||
parts.append(exc.__class__.__name__)
|
||||
blob = " ".join(parts).lower()
|
||||
blob = _error_blob(exc, error_text)
|
||||
if not blob.strip():
|
||||
return "unknown"
|
||||
if "message_too_long" in blob or "too long" in blob or "message is too long" in blob:
|
||||
|
|
@ -1988,7 +2002,7 @@ def classify_send_error(exc: Optional[BaseException], error_text: str = "") -> s
|
|||
return "unknown"
|
||||
|
||||
|
||||
def is_chat_level_not_found(error_text: str = "", exc: Optional[BaseException] = None) -> bool:
|
||||
def is_chat_level_not_found(exc: Optional[BaseException] = None, error_text: str = "") -> bool:
|
||||
"""Whether a ``not_found`` failure means the *whole chat* is gone.
|
||||
|
||||
:func:`classify_send_error` collapses chat-level and thread/topic/message-level
|
||||
|
|
@ -1997,13 +2011,12 @@ def is_chat_level_not_found(error_text: str = "", exc: Optional[BaseException] =
|
|||
forum topic or an edited-away message leaves the parent chat reachable. When
|
||||
both a chat-level and a sub-chat marker are present, the sub-chat reading wins
|
||||
(conservative: never kill a chat that may still be reachable).
|
||||
|
||||
Argument order mirrors :func:`classify_send_error` (``exc`` first) and both
|
||||
share :func:`_error_blob`, so the two classifiers cannot disagree on the same
|
||||
failure.
|
||||
"""
|
||||
parts = []
|
||||
if error_text:
|
||||
parts.append(error_text)
|
||||
if exc is not None:
|
||||
parts.append(str(exc))
|
||||
blob = " ".join(parts).lower()
|
||||
blob = _error_blob(exc, error_text)
|
||||
if any(s in blob for s in _SUBCHAT_NOT_FOUND_SUBSTRINGS):
|
||||
return False
|
||||
return any(s in blob for s in _CHAT_LEVEL_NOT_FOUND_SUBSTRINGS)
|
||||
|
|
|
|||
|
|
@ -225,19 +225,19 @@ class TestNotFoundBlastRadius:
|
|||
def test_is_chat_level_not_found_chat_level(self):
|
||||
from gateway.platforms.base import is_chat_level_not_found
|
||||
|
||||
assert is_chat_level_not_found("Bad Request: chat not found") is True
|
||||
assert is_chat_level_not_found(error_text="Bad Request: chat not found") is True
|
||||
|
||||
@pytest.mark.parametrize("message", _SUBCHAT_NOT_FOUND_MESSAGES)
|
||||
def test_is_chat_level_not_found_subchat(self, message):
|
||||
from gateway.platforms.base import is_chat_level_not_found
|
||||
|
||||
assert is_chat_level_not_found(message) is False
|
||||
assert is_chat_level_not_found(error_text=message) is False
|
||||
|
||||
def test_subchat_marker_wins_when_both_present(self):
|
||||
from gateway.platforms.base import is_chat_level_not_found
|
||||
|
||||
# Conservative: if a sub-chat marker is present, never kill the whole chat.
|
||||
assert is_chat_level_not_found("chat not found; message thread not found") is False
|
||||
assert is_chat_level_not_found(error_text="chat not found; message thread not found") is False
|
||||
|
||||
def test_classify_dead_from_error_text_gates_not_found(self):
|
||||
from gateway.delivery import _classify_dead_from_error_text
|
||||
|
|
@ -246,3 +246,19 @@ class TestNotFoundBlastRadius:
|
|||
assert _classify_dead_from_error_text("Bad Request: chat not found") == "not_found"
|
||||
assert _classify_dead_from_error_text("Bad Request: message thread not found") is None
|
||||
assert _classify_dead_from_error_text("httpx.ReadTimeout: connection timed out") is None
|
||||
|
||||
def test_error_blob_is_shared_source_of_truth(self):
|
||||
# Regression guard: classify_send_error and is_chat_level_not_found must
|
||||
# both derive their match text from the SAME _error_blob helper (which
|
||||
# includes the exception CLASS NAME), so they can never drift. Before
|
||||
# this consolidation is_chat_level_not_found built its own blob from
|
||||
# str(exc) only, omitting the class name classify_send_error included.
|
||||
from gateway.platforms import base
|
||||
|
||||
class TopicDeleted(Exception):
|
||||
pass
|
||||
|
||||
# Empty message: the only signal is the class name — _error_blob keeps it,
|
||||
# with no stray leading space from an empty str(exc).
|
||||
assert base._error_blob(TopicDeleted()) == "topicdeleted"
|
||||
assert base._error_blob(TopicDeleted("boom")) == "boom topicdeleted"
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue