mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
3af80907c7
The initial-message worker (_run_initial) collapsed both cancel and backend-error exits into one `except (Exception, GenerationCancelled)` arm that always stamped state=idle, clobbering the state=error that session.send's _record_fatal_error had persisted+emitted. A spawned child's first-turn backend failure (unreachable model server, exhausted quota, auth error) therefore read as an empty, successful turn — the coordinator's wait/inspect surface reads last_error only for state=='error' — and the real error surfaced only after a manual nudge re-ran the turn synchronously. Split the arm: cancel -> idle, exception -> error. The failed child now settles at state=error and the first wait_for_workstream returns the enriched backend error inline. Also fixes the same latent bug for scheduled tasks, which dispatch through the same endpoint and closure. A failed first turn is deliberately terminal for automated wakes: it settles to a non-ready error terminal, not the idle ready-set that timer/watch wakes recur to, so explicit user/coordinator action reactivates it rather than a silent auto-retry (a self-healing wake-from-error would be a separate wake-gate change). The exception arm routes through a new ChatSession.ensure_error_recorded: a no-op when send already recorded the error in-line (the common backend-boundary path — no duplicate state emit), and the recorder when a pre-try exception (model-registry refresh, user-turn append, system-message recompose) bypassed send's own handler, so state=error always carries a meaningful last_error. Its idempotency guard (_has_persisted_error) is session-lifetime, so ensure_error_recorded is scoped to _run_initial's FRESH first-turn session only; the docstring spells out why a session-reuse caller (retry, /send, coord send, wake) must not route through it until the per-turn error-recorded signal of #865 lands. The other half of making an errored workstream cheap for a model to handle is a stable identifier: the enriched backend error now leads with the model ALIAS the coordinator references everywhere (list_nodes, spawn) and annotates the backend id for the operator — "model=DeepSeek-V4-Flash (id=deepseek-v4-flash)" — so a model routing around a failed model correlates it against those surfaces without a lookup, instead of burning reasoning tokens reconciling the alias against a backend id it never sees anywhere else. Collapses to one token when the alias and id coincide. Tests (TestInitialWorkerFailureState) assert the coordinator-visible manager state and the persisted last_error across the matrix — common- backend and pre-try errors both settle error with a readable last_error; cancel-to-idle settles idle with no error recorded. De-forks the create-app fixture and uses the shared monotonic wait_until helper. The completion-notification honesty surface and the error-recording hygiene of the other send-worker closures (retry / main send / coord send / wake) are deferred to #865.
367 lines
12 KiB
Python
367 lines
12 KiB
Python
"""Tests for :meth:`ChatSession._format_backend_error`.
|
|
|
|
The helper turns bare backend-boundary exceptions (httpx ``ReadTimeout``,
|
|
OpenAI SDK ``APITimeoutError`` / ``APIConnectionError`` /
|
|
``NotFoundError`` / ``RateLimitError`` / ``AuthenticationError``) into
|
|
operator-actionable messages that include the provider, base URL, and
|
|
model. We bind the method to lightweight stubs rather than constructing
|
|
a full :class:`ChatSession`: the helper only reads ``self.client``,
|
|
``self._provider``, ``self.model``, and ``self._model_alias``, so a
|
|
SimpleNamespace stub exercises the same surface without dragging in the
|
|
storage / prompt composition fixtures.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
from types import SimpleNamespace
|
|
from typing import Any
|
|
|
|
import pytest
|
|
|
|
from turnstone.core.session import ChatSession
|
|
|
|
|
|
def _stub(
|
|
*,
|
|
base_url: str = "http://192.168.0.5:8000/v1",
|
|
provider_name: str = "openai-compatible",
|
|
model: str = "flatspark",
|
|
model_alias: str | None = "flatspark",
|
|
client_attr: str = "base_url",
|
|
) -> Any:
|
|
"""Build a minimal session-like stub for ``_format_backend_error``.
|
|
|
|
``client_attr`` selects which attribute on the client carries the
|
|
URL — both ``base_url`` (OpenAI / Anthropic SDK public surface) and
|
|
``_base_url`` (httpx fallback) are exercised by the helper.
|
|
"""
|
|
client_kwargs: dict[str, Any] = {client_attr: base_url}
|
|
return SimpleNamespace(
|
|
client=SimpleNamespace(**client_kwargs),
|
|
_provider=SimpleNamespace(provider_name=provider_name),
|
|
model=model,
|
|
_model_alias=model_alias,
|
|
)
|
|
|
|
|
|
def _format(stub: Any, exc: BaseException) -> str | None:
|
|
"""Invoke the method as if on a real session — ``__func__`` skips
|
|
the descriptor protocol so we can pass any object as ``self``."""
|
|
return ChatSession._format_backend_error(stub, exc) # type: ignore[arg-type]
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Synthetic exception classes — class name is what the helper matches on,
|
|
# so we don't need real httpx / openai imports here.
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
# N818 (Error suffix on Exception names) is intentionally suppressed
|
|
# for the four classes below — they exist to impersonate httpx /
|
|
# Anthropic SDK exception class names verbatim, since the formatter
|
|
# matches by class name. Renaming them defeats the test.
|
|
|
|
|
|
class ReadTimeout(Exception): # noqa: N818
|
|
pass
|
|
|
|
|
|
class WriteTimeout(Exception): # noqa: N818
|
|
pass
|
|
|
|
|
|
class APITimeoutError(Exception):
|
|
pass
|
|
|
|
|
|
class ConnectError(Exception): # noqa: N818
|
|
pass
|
|
|
|
|
|
class ConnectTimeout(Exception): # noqa: N818
|
|
pass
|
|
|
|
|
|
class APIConnectionError(Exception):
|
|
pass
|
|
|
|
|
|
class NotFoundError(Exception):
|
|
pass
|
|
|
|
|
|
class AuthenticationError(Exception):
|
|
pass
|
|
|
|
|
|
class PermissionDeniedError(Exception):
|
|
pass
|
|
|
|
|
|
class RateLimitError(Exception):
|
|
pass
|
|
|
|
|
|
class SomeUnrelatedError(Exception):
|
|
"""Outside the recognised set — should fall through to ``None``."""
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Known categories — each branch produces an operator-actionable message
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@pytest.mark.parametrize("exc_cls", [ReadTimeout, WriteTimeout, APITimeoutError])
|
|
def test_timeout_message_names_backend_and_model(exc_cls):
|
|
msg = _format(_stub(), exc_cls())
|
|
assert msg is not None
|
|
assert "Backend timeout" in msg
|
|
assert exc_cls.__name__ in msg
|
|
assert "openai-compatible" in msg
|
|
assert "http://192.168.0.5:8000/v1" in msg
|
|
assert "model=flatspark" in msg
|
|
assert "wedged" in msg
|
|
|
|
|
|
@pytest.mark.parametrize("exc_cls", [ConnectError, ConnectTimeout, APIConnectionError])
|
|
def test_connect_message_says_unreachable(exc_cls):
|
|
msg = _format(_stub(), exc_cls("dial tcp: i/o timeout"))
|
|
assert msg is not None
|
|
assert "Backend unreachable" in msg
|
|
assert exc_cls.__name__ in msg
|
|
assert "http://192.168.0.5:8000/v1" in msg
|
|
# Raw exception text is preserved as a tail for grep-correlation.
|
|
assert "dial tcp: i/o timeout" in msg
|
|
|
|
|
|
def test_not_found_points_at_model_name_mismatch():
|
|
msg = _format(_stub(model="flatspark"), NotFoundError("model flatspark not found"))
|
|
assert msg is not None
|
|
assert "Backend reports model not loaded" in msg
|
|
assert "no model named 'flatspark'" in msg
|
|
assert "/v1/models" in msg # operator hint
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Model label — lead with the alias the model references, annotate the id
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def test_error_leads_with_alias_and_annotates_backend_id():
|
|
"""When the display alias differs from the backend model id, the enriched
|
|
error leads with the ALIAS (the identifier the model references everywhere —
|
|
list_nodes, spawn) and annotates the backend id for the operator, so a
|
|
coordinator correlates the failure with those surfaces without a lookup."""
|
|
msg = _format(
|
|
_stub(model="deepseek-v4-flash", model_alias="DeepSeek-V4-Flash"),
|
|
APIConnectionError("cannot reach"),
|
|
)
|
|
assert msg is not None
|
|
assert "model=DeepSeek-V4-Flash (id=deepseek-v4-flash)" in msg
|
|
|
|
|
|
def test_error_model_label_collapses_when_alias_equals_id():
|
|
"""No redundant (id=...) annotation when the alias and backend id coincide."""
|
|
msg = _format(_stub(model="flatspark", model_alias="flatspark"), APIConnectionError("x"))
|
|
assert msg is not None
|
|
assert "model=flatspark" in msg
|
|
assert "(id=" not in msg
|
|
|
|
|
|
@pytest.mark.parametrize("exc_cls", [AuthenticationError, PermissionDeniedError])
|
|
def test_auth_message_mentions_api_key(exc_cls):
|
|
msg = _format(_stub(), exc_cls("invalid api key"))
|
|
assert msg is not None
|
|
assert "Backend rejected credentials" in msg
|
|
assert "API key" in msg
|
|
|
|
|
|
def test_rate_limit_message():
|
|
msg = _format(_stub(), RateLimitError("limit exceeded"))
|
|
assert msg is not None
|
|
assert "Backend rate-limited" in msg
|
|
assert "limit exceeded" in msg
|
|
|
|
|
|
def test_rate_limit_with_overflow_phrasing_is_not_mislabeled_overflow():
|
|
"""A recognized RateLimitError whose quota text happens to contain a
|
|
context-overflow phrase must still render as rate-limited — the text-based
|
|
overflow branch is gated on 'not a known class', so it can't hijack a
|
|
recognized error and mark a transient 429 as a hard 'Context window exceeded'."""
|
|
msg = _format(
|
|
_stub(), RateLimitError("exceeds the maximum number of tokens allowed per minute")
|
|
)
|
|
assert msg is not None
|
|
assert "Backend rate-limited" in msg
|
|
assert "Context window exceeded" not in msg
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Fall-through + degradation behaviour
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def test_unknown_exception_returns_none():
|
|
assert _format(_stub(), SomeUnrelatedError("anything")) is None
|
|
|
|
|
|
def test_unknown_exception_value_error_returns_none():
|
|
assert _format(_stub(), ValueError("not a backend error")) is None
|
|
|
|
|
|
def test_trailing_slash_and_query_string_stripped():
|
|
msg = _format(
|
|
_stub(base_url="http://node-a:8000/v1/?api_key=secret&foo=1"),
|
|
ReadTimeout(),
|
|
)
|
|
assert msg is not None
|
|
assert "http://node-a:8000/v1" in msg
|
|
# Query string (which may carry credentials) is stripped before the
|
|
# message is built — sanitize_error_text is a second line of defence
|
|
# but the helper itself must not embed query params verbatim.
|
|
assert "api_key" not in msg
|
|
assert "secret" not in msg
|
|
|
|
|
|
def test_missing_provider_degrades_to_placeholder():
|
|
stub = _stub()
|
|
stub._provider = None
|
|
msg = _format(stub, ReadTimeout())
|
|
assert msg is not None
|
|
# No exception, no NoneType formatting leaking through.
|
|
assert "Backend timeout" in msg
|
|
assert "from ?" in msg or "openai-compatible" not in msg
|
|
|
|
|
|
def test_client_base_url_raises_degrades_gracefully():
|
|
class _BadClient:
|
|
@property
|
|
def base_url(self) -> str:
|
|
raise RuntimeError("boom")
|
|
|
|
stub = SimpleNamespace(
|
|
client=_BadClient(),
|
|
_provider=SimpleNamespace(provider_name="openai-compatible"),
|
|
model="flatspark",
|
|
_model_alias="flatspark",
|
|
)
|
|
msg = _format(stub, ReadTimeout())
|
|
assert msg is not None
|
|
assert "Backend timeout" in msg
|
|
# base_url accessor blew up — message still renders with placeholder.
|
|
assert "at ?" in msg
|
|
|
|
|
|
def test_httpx_underscore_base_url_fallback():
|
|
# httpx client carries ``_base_url`` on some versions instead of
|
|
# ``base_url`` — the helper checks both.
|
|
stub = _stub(base_url="http://alt-host:9000", client_attr="_base_url")
|
|
# SimpleNamespace exposes the attr; remove the public one so the
|
|
# fallback path is exercised.
|
|
delattr(stub.client, "base_url") if hasattr(stub.client, "base_url") else None
|
|
msg = _format(stub, ReadTimeout())
|
|
assert msg is not None
|
|
assert "http://alt-host:9000" in msg
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Integration with _record_fatal_error — original bare-class string is
|
|
# replaced by the enriched message when the exception type is recognised.
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def _record_fatal_stub(ui: Any, captured: dict[str, str]) -> Any:
|
|
"""Build a stub for the ``_record_fatal_error`` integration tests.
|
|
|
|
``_record_fatal_error`` calls ``self._format_backend_error(...)``
|
|
internally, so the stub binds the unbound method to itself rather
|
|
than relying on Python's descriptor protocol (which only kicks in
|
|
when ``self`` is a real instance of the class)."""
|
|
stub = SimpleNamespace(
|
|
client=SimpleNamespace(base_url="http://192.168.0.5:8000/v1"),
|
|
_provider=SimpleNamespace(provider_name="openai-compatible"),
|
|
model="flatspark",
|
|
_model_alias="flatspark",
|
|
_ws_id="ws-test",
|
|
_has_persisted_error=False,
|
|
ui=ui,
|
|
_emit_state=lambda state: captured.setdefault("state", state),
|
|
)
|
|
stub._format_backend_error = lambda exc: ChatSession._format_backend_error(stub, exc)
|
|
return stub
|
|
|
|
|
|
def test_record_fatal_uses_enriched_message_for_known(monkeypatch):
|
|
"""End-to-end: a recognised exception flows through
|
|
``_record_fatal_error`` and the enriched text reaches both the UI
|
|
and the persist hook."""
|
|
|
|
captured: dict[str, str] = {}
|
|
|
|
def fake_persist(ws_id: str, msg: str) -> None:
|
|
captured["persist"] = msg
|
|
|
|
def fake_sanitize(text: str, *, max_len: int = 1024) -> str:
|
|
# Skip the credential-redaction module (and its module-level
|
|
# regex compile) by returning the input verbatim — the helper
|
|
# under test produces no credentials.
|
|
return text
|
|
|
|
import turnstone.core.memory as memory_mod
|
|
|
|
monkeypatch.setattr(memory_mod, "persist_last_error", fake_persist)
|
|
monkeypatch.setattr(memory_mod, "sanitize_error_text", fake_sanitize)
|
|
|
|
class _UI:
|
|
def __init__(self) -> None:
|
|
self.errors: list[str] = []
|
|
|
|
def on_error(self, msg: str) -> None:
|
|
self.errors.append(msg)
|
|
|
|
ui = _UI()
|
|
stub = _record_fatal_stub(ui, captured)
|
|
|
|
ChatSession._record_fatal_error(stub, ReadTimeout()) # type: ignore[arg-type]
|
|
|
|
assert ui.errors, "UI never received error"
|
|
assert "Backend timeout" in ui.errors[0]
|
|
assert "ReadTimeout" in ui.errors[0]
|
|
assert captured["persist"] == ui.errors[0]
|
|
assert captured["state"] == "error"
|
|
assert stub._has_persisted_error is True
|
|
|
|
|
|
def test_record_fatal_falls_back_for_unknown(monkeypatch):
|
|
"""An unrecognised exception keeps the legacy
|
|
``f"{type(exc).__name__}: {exc}"`` shape so we don't regress
|
|
existing call sites that grep on it."""
|
|
|
|
captured: dict[str, str] = {}
|
|
|
|
def fake_persist(ws_id: str, msg: str) -> None:
|
|
captured["persist"] = msg
|
|
|
|
def fake_sanitize(text: str, *, max_len: int = 1024) -> str:
|
|
return text
|
|
|
|
import turnstone.core.memory as memory_mod
|
|
|
|
monkeypatch.setattr(memory_mod, "persist_last_error", fake_persist)
|
|
monkeypatch.setattr(memory_mod, "sanitize_error_text", fake_sanitize)
|
|
|
|
class _UI:
|
|
def __init__(self) -> None:
|
|
self.errors: list[str] = []
|
|
|
|
def on_error(self, msg: str) -> None:
|
|
self.errors.append(msg)
|
|
|
|
ui = _UI()
|
|
stub = _record_fatal_stub(ui, captured)
|
|
|
|
ChatSession._record_fatal_error(stub, ValueError("plain old error")) # type: ignore[arg-type]
|
|
|
|
assert ui.errors == ["ValueError: plain old error"]
|
|
assert captured["persist"] == "ValueError: plain old error"
|