From 0a5b97ec44c33c9781988c66b3b3c6a3bc4e2462 Mon Sep 17 00:00:00 2001 From: Oranje AI <293843428+oranjeai@users.noreply.github.com> Date: Tue, 4 Aug 2026 09:43:19 +1200 Subject: [PATCH] memory: never report a failed recall as "no relevant memories" A memory_recall whose embedding call fails at the connection layer -- DNS, refused, reset, or a transport closed under it by an LLM-client reset -- rendered as "Recalled 0 memories" / "No relevant memories found.". A session then concludes memory is empty, skips its recall obligation and redoes work: the confident wrong answer MemoryBackendUnavailable was introduced to prevent (835d857). Not theoretical. "Connection error." appears 42 times in this instance's 1.95 GB nerve.log (measured 2026-08-03 21:10 UTC; rolling), every one on the memorize/chat side and 0 on "memU recall failed" -- so the exception class occurs routinely in production and recall has simply not been the unlucky caller yet. Unexercised, not latent. Root cause: recall()'s classification enumerated transient *signatures* instead of partitioning failure *kinds*. Its only term, _is_transient_llm_error, keys on an HTTP status (429/5xx, plus auth on 401/403), so a call that fails at the connection layer carries no status and cannot match it by construction; it fell through to `return []` alongside genuine logic errors. Nothing normalizes it on the way up either: memu/app/retrieve.py:42 retrieve has 0 except arms (verified by AST), so the raw SDK exception reaches nerve's handler unwrapped. The tell that this is an inconsistent cut across the transport hierarchies rather than a missing exception type: openai.APITimeoutError is a subclass of openai.APIConnectionError, and the code already treated that as backend-down. It accepted a proper subset of the hierarchy and rejected the rest of it. The fix makes `[]` from recall() mean "retrieval returned nothing", for every failure recall() can observe: * a new _is_llm_transport_failure tests each SDK's APIConnectionError base class. Both openai and anthropic are needed and neither subsumes the other (measured: issubclass(anthropic.APIConnectionError, openai.APIConnectionError) is False), because memU's own client raises the former while nerve's _BedrockLLMClient raises the latter. Imports are guarded so the module stays importable without a given provider, and the guard wraps only the import, never the isinstance -- a predicate bug must surface, not degrade to []. * infrastructure failures raise MemoryBackendUnavailable; everything else propagates to the caller's pre-existing generic error path instead of being swallowed. All three callers already catch, so the inversion cannot escape unhandled. * the uninitialized-bridge guard raises instead of returning [] -- the same lie by a different route, with no exception involved at all. * the tool message drops its false cause attributions -- "transient proxy/auth error" and the closing "memory is down" -- for outcome-only wording, since the same arm covers 429s, which *are* responses, and both a self-closed local transport and a bridge that never initialized leave the remote blameless. The class docstring is likewise rewritten around the outcome: its LLM-only enumeration was already false, because memorize_file raises the same type for SQLite write-lock contention. A genuine miss still returns [], pinned by a test, else the fix would make every miss look like an outage. engine.py's pre-recall additionally stops freezing a failed recall into session metadata, where it was replayed on every rebuild. Scoped deliberately to recall(). memorize_file, update_item and the two category paths keep their current returns: their tool layers already render an explicit failure, so they produce no confident wrong *answer*, and a write reporting failure is honest -- only a read reporting emptiness lies. Of the 50 broad except arms in this file, exactly two read paths still return [] after this change (list_items, list_categories), and both are non-network-bearing, verified rather than assumed: within those two callee function bodies there are 0 embed/llm/client references (the claim is function-scoped; whole-file crud.py has many), against 5 embed_client.embed call sites in retrieve. The invariant is stated scoped, not global: no failure that *reaches* MemUBridge.recall is reported as []. It is deliberately not "[] always means a successful miss", because memU absorbs a malformed LLM ranking response below this arm (memu/app/retrieve.py:1341-1347 wraps the parse in `except Exception -> logger.warning` and returns an empty list; memu-py==1.4.0 is pinned), so a garbled ranker reply still yields a legitimate empty recall with no exception reaching us. Closing that is a memU-layer change. Tests: 15 new arms. Both directions -- base source with the new tests kept gives 14 failed / 1 passed (the 1 is the genuine-miss control, which must hold on both trees), the fix gives 15 passed. Two arms drive the real production callers rather than a reconstruction: the engine arm calls _get_or_create_client and asserts the persisted session metadata, so the freezing claim above is pinned by the actual DB write, and the session_context arm asserts its own handler renders the error. A 17-mutant matrix kills every mutant with the unmutated control green at both ends, including the two that discriminate under- from over-classification (restoring `return []` vs raising MemoryBackendUnavailable for a ValueError), and the one that narrows the predicate to a __cause__-message check, which fixes the 0-occurrence closed-transport sub-case while leaving the 42-occurrence one broken. The two mutants covering the real callers were each measured to survive the earlier, reconstructed test shape and to be killed by the current one, so the coverage those two arms add is demonstrated rather than asserted. Three further mutants were measured the same way: one that skips the engine's pre-recall branch entirely, one that returns [] without ever calling retrieve, and one that re-asserts the "memory is down" cause. Each passed the earlier tests -- the first two because "no metadata written" and "== []" are also the outcomes of a path that never ran, the third because the oracle's needle was case-sensitive -- and each is killed now, so every assertion added here is pinned by a measurement rather than by argument. Reverting the class docstring is a documented predicted survival: a docstring has no runtime observer. Whole suite 2933 -> 2948 passed with an identical failed-test name set (7 pre-existing timezone failures, base measured in a clean worktree at origin/main); ruff reports 79 findings at base and at head with identical finding sets. --- nerve/agent/tools/handlers/memory.py | 6 +- nerve/memory/memu_bridge.py | 60 ++++- tests/test_memu_bridge.py | 330 +++++++++++++++++++++++++++ 3 files changed, 382 insertions(+), 14 deletions(-) diff --git a/nerve/agent/tools/handlers/memory.py b/nerve/agent/tools/handlers/memory.py index fe36b3b9..23a5d99b 100644 --- a/nerve/agent/tools/handlers/memory.py +++ b/nerve/agent/tools/handlers/memory.py @@ -107,9 +107,9 @@ async def memory_recall_handler(ctx: ToolContext, args: dict) -> ToolResult: except MemoryBackendUnavailable as e: logger.warning("Memory recall: backend unavailable: %s", e) memu_block = ( - "⚠️ MEMORY BACKEND DOWN (transient proxy/auth error) — recall is UNAVAILABLE right now, " - "NOT empty. Do NOT conclude 'no relevant memories'. Reconstruct context from other " - "sources (git history, your own notes) and alert the operator that memory is down." + "⚠️ MEMORY RECALL UNAVAILABLE — this recall did not complete, so this is NOT an " + "empty result. Do NOT conclude 'no relevant memories'. Reconstruct context from other " + "sources (git history, your own notes) and alert the operator that this recall failed." ) except Exception as e: logger.error("Memory recall failed: %s", e) diff --git a/nerve/memory/memu_bridge.py b/nerve/memory/memu_bridge.py index 9642175b..6c347e15 100644 --- a/nerve/memory/memu_bridge.py +++ b/nerve/memory/memu_bridge.py @@ -37,11 +37,15 @@ class MemoryBackendUnavailable(RuntimeError): - """Raised when a memU operation fails because the LLM backend (the shared - proxy route) is transiently unavailable — HTTP 429/5xx or an auth blip - (``auth_unavailable`` / revoked OAuth) — rather than because the query - genuinely had no results. The tool layer surfaces this LOUDLY so a session - never mistakes "memory is DOWN" for "no relevant memories".""" + """Raised when a memU operation could not complete because the backend was + unavailable, rather than because the query genuinely had no results. The + tool layer surfaces this LOUDLY so a session never mistakes "memory is + DOWN" for "no relevant memories". + + Deliberately described by OUTCOME, not by cause: the raising sites already + cover an HTTP 429/5xx or auth blip, an LLM-call timeout, a transport-level + failure with no HTTP response, SQLite write-lock contention, and a bridge + that never initialized. Enumerating causes here goes stale.""" def _is_transient_llm_error(exc: BaseException) -> bool: @@ -72,6 +76,34 @@ def _is_transient_llm_error(exc: BaseException) -> bool: return "auth_unavailable" in low +def _is_llm_transport_failure(exc: BaseException) -> bool: + """True if ``exc`` is a transport-level LLM failure: the call failed at the + connection layer rather than with an HTTP status (DNS, refused, reset, + closed transport; possibly mid-response). + + Disjoint from :func:`_is_transient_llm_error`, which keys on an HTTP + *status*. Both mean the call produced no usable result, so recall must not + report "no results". + + Both SDK families are needed and neither subsumes the other: memU's own + client raises ``openai.APIConnectionError``, ``_BedrockLLMClient`` raises + ``anthropic.APIConnectionError``. Imports are guarded so the module stays + importable without a given provider. Each SDK's ``APITimeoutError`` + subclasses its ``APIConnectionError``, so the base-class test covers + timeouts; raw ``httpx`` errors need no arm because both SDKs wrap them.""" + for _mod, _name in ( + ("openai", "APIConnectionError"), + ("anthropic", "APIConnectionError"), + ): + try: + _exc_type = getattr(__import__(_mod), _name) + except (ImportError, AttributeError): + continue + if isinstance(exc, _exc_type): + return True + return False + + def _is_sqlite_locked_error(exc: BaseException) -> bool: """True if ``exc`` is SQLite lock contention — worth retrying. @@ -2786,9 +2818,14 @@ async def recall( Args: limit: max item hits to return (full content). category_limit: max category breadcrumbs to return. + + Raises: + MemoryBackendUnavailable: the backend could not be reached, or the + bridge never initialized. Any other exception propagates. + An empty list therefore means retrieval returned nothing. """ if not self._available or not self._service: - return [] + raise MemoryBackendUnavailable("memory backend is not initialized") op_id = self._metrics.begin_op("recall", query[:80]) try: # Retrieve runs vector search (numpy) and possibly SQLite reads @@ -2846,12 +2883,13 @@ async def recall( except Exception as e: logger.error("memU recall failed: %s", e) self._metrics.end_op(op_id, success=False, error=str(e)) - # A transient backend outage must not masquerade as "no results" — - # surface it so the caller can flag amnesia instead of trusting the - # empty list. - if _is_transient_llm_error(e): + # Infrastructure failures get the dedicated type; anything else + # propagates instead of being swallowed. (memU can still absorb a + # malformed ranking reply below this level and return a real `[]`.) + if (isinstance(e, TimeoutError) or _is_transient_llm_error(e) + or _is_llm_transport_failure(e)): raise MemoryBackendUnavailable(f"memory backend unavailable: {e}") from e - return [] + raise async def expand_category( self, category_id: str, query: str = "", limit: int = 20, diff --git a/tests/test_memu_bridge.py b/tests/test_memu_bridge.py index 92037666..aed607dc 100644 --- a/tests/test_memu_bridge.py +++ b/tests/test_memu_bridge.py @@ -7,9 +7,13 @@ from pathlib import Path from unittest.mock import AsyncMock, MagicMock, patch +import httpx +import openai import pytest import pytest_asyncio +from nerve.agent.tools.handlers.memory import memory_recall_handler +from nerve.agent.tools.registry import ToolContext from nerve.config import MemoryConfig, NerveConfig from nerve.memory.memu_bridge import ( MemoryBackendUnavailable, @@ -1134,3 +1138,329 @@ async def test_transient_llm_error_still_raises_backend_unavailable(self, tmp_pa await bridge.memorize_file(str(target)) assert bridge._service.memorize.await_count == 1 + + +def _make_recall_bridge(tmp_path: Path) -> MemUBridge: + """Bridge wired for recall() tests: available, mocked service, no memU loop + (so _submit awaits inline).""" + bridge = MemUBridge(_make_config(tmp_path)) + bridge._available = True + bridge._service = MagicMock() + return bridge + + +def _recall_ctx(bridge) -> ToolContext: + return ToolContext( + session_id="s-recall", + workspace=Path("/tmp/ws"), + db=None, + memory_bridge=bridge, + config=None, + ) + + +_REQ = httpx.Request("POST", "http://embeddings.invalid/v1/embeddings") + + +def _conn_error(cause: BaseException | None = None) -> openai.APIConnectionError: + """The exception the OpenAI SDK raises when the call never got a response.""" + exc = openai.APIConnectionError(request=_REQ) + if cause is not None: + exc.__cause__ = cause + return exc + + +class TestRecallTransportFailureClassification: + """recall() must never report a FAILURE as "no results". + + A transport-level LLM failure carries no HTTP status, so + _is_transient_llm_error cannot match it by construction; before this class + existed such a failure returned [] and the tool layer rendered it as + "Recalled 0 memories" -- a confident wrong answer. An empty list from + recall() now means retrieval genuinely returned nothing. + """ + + # --- infrastructure failures => MemoryBackendUnavailable --------------- + + @pytest.mark.asyncio + async def test_t1_connection_error_raises_backend_unavailable(self, tmp_path): + bridge = _make_recall_bridge(tmp_path) + bridge._service.retrieve = AsyncMock(side_effect=_conn_error()) + + with pytest.raises(MemoryBackendUnavailable, match="unavailable"): + await bridge.recall("q") + + @pytest.mark.asyncio + async def test_t2_closed_transport_raises_backend_unavailable(self, tmp_path): + """The reset window: _reset_llm_clients_impl closed the transport under + an in-flight call.""" + bridge = _make_recall_bridge(tmp_path) + bridge._service.retrieve = AsyncMock(side_effect=_conn_error( + RuntimeError("Cannot send a request, as the client has been closed."), + )) + + with pytest.raises(MemoryBackendUnavailable, match="unavailable"): + await bridge.recall("q") + + @pytest.mark.asyncio + async def test_t3_refused_connection_raises_backend_unavailable(self, tmp_path): + """The shape the SDK actually produces for a refused connection -- the + 27+ production occurrences. A BARE httpx.ConnectError is deliberately + NOT classified (no carrier: nerve sets client_backend="sdk"), so this + arm uses the wrapped form the SDK really raises.""" + bridge = _make_recall_bridge(tmp_path) + bridge._service.retrieve = AsyncMock(side_effect=_conn_error( + httpx.ConnectError("[Errno 111] Connection refused"), + )) + + with pytest.raises(MemoryBackendUnavailable, match="unavailable"): + await bridge.recall("q") + + @pytest.mark.asyncio + async def test_t7_anthropic_connection_error_raises_backend_unavailable(self, tmp_path): + """The Bedrock family. anthropic.APIConnectionError is NOT a subclass of + openai's, so a single-SDK predicate would miss nerve's own + _BedrockLLMClient -- this arm is what pins two-family coverage.""" + anthropic = pytest.importorskip("anthropic") + assert not issubclass( + anthropic.APIConnectionError, openai.APIConnectionError, + ), "families are expected to be disjoint; the predicate needs both arms" + + bridge = _make_recall_bridge(tmp_path) + bridge._service.retrieve = AsyncMock( + side_effect=anthropic.APIConnectionError(request=_REQ), + ) + + with pytest.raises(MemoryBackendUnavailable, match="unavailable"): + await bridge.recall("q") + + @pytest.mark.asyncio + async def test_t10_plain_asyncio_timeout_raises_backend_unavailable(self, tmp_path): + """asyncio.wait_for expiry is builtins.TimeoutError, matched by none of + the SDK predicates. _instrument_llm_timeouts wraps .chat() in + asyncio.wait_for for the "fast" profile, which is the profile recall's + LLM ranker uses, so this is reachable on recall's own path.""" + assert asyncio.TimeoutError is TimeoutError + + bridge = _make_recall_bridge(tmp_path) + bridge._service.retrieve = AsyncMock(side_effect=TimeoutError("timed out")) + + with pytest.raises(MemoryBackendUnavailable, match="unavailable"): + await bridge.recall("q") + + @pytest.mark.asyncio + async def test_t11_api_timeout_error_raises_backend_unavailable(self, tmp_path): + """APITimeoutError is a SUBCLASS of APIConnectionError, which is why the + arm does not need to call _is_llm_timeout. If a future SDK reparents it, + this arm fails loudly instead of silently losing coverage.""" + assert issubclass(openai.APITimeoutError, openai.APIConnectionError) + + bridge = _make_recall_bridge(tmp_path) + bridge._service.retrieve = AsyncMock( + side_effect=openai.APITimeoutError(request=_REQ), + ) + + with pytest.raises(MemoryBackendUnavailable, match="unavailable"): + await bridge.recall("q") + + # --- everything else propagates unchanged ----------------------------- + + @pytest.mark.asyncio + async def test_t4_logic_error_re_raises_as_itself(self, tmp_path): + """A logic error is neither disguised as a backend outage nor as an + empty result: it propagates to the caller's generic error path. + + NOTE this deliberately INVERTS the memorize_file control shape + (test_non_transient_error_still_fails_fast asserts ok is False): + memorize_file returns a bool, recall returns results, and [] is a + meaningful value there.""" + bridge = _make_recall_bridge(tmp_path) + bridge._service.retrieve = AsyncMock(side_effect=ValueError("bad payload")) + + with pytest.raises(ValueError, match="bad payload"): + await bridge.recall("q") + + @pytest.mark.asyncio + async def test_t14_genuine_miss_still_returns_empty(self, tmp_path): + """The arm that keeps the inversion honest: [] must still be reachable + for a successful retrieval with no hits, else every miss would look + like an outage.""" + bridge = _make_recall_bridge(tmp_path) + bridge._service.retrieve = AsyncMock( + return_value={"items": [], "categories": []}, + ) + + assert await bridge.recall("q") == [] + assert bridge._service.retrieve.await_count == 1, ( + "retrieval was skipped: [] would not prove a genuine miss" + ) + assert bridge._service.retrieve.await_args.kwargs["queries"] == [ + {"role": "user", "content": "q"}, + ] + + # --- the uninitialized-bridge guard ----------------------------------- + + @pytest.mark.asyncio + async def test_t12_uninitialized_bridge_raises_backend_unavailable(self, tmp_path): + """The same lie by a different route: a bridge that never initialized + used to answer [] with no exception involved at all.""" + bridge = MemUBridge(_make_config(tmp_path)) + bridge._available = False + + with pytest.raises(MemoryBackendUnavailable, match="not initialized"): + await bridge.recall("q") + + # --- the rendered artifact (what the agent actually reads) ------------- + + @pytest.mark.asyncio + async def test_t6_handler_renders_unavailable_not_a_count(self, tmp_path): + bridge = _make_recall_bridge(tmp_path) + bridge._service.retrieve = AsyncMock(side_effect=_conn_error()) + + result = await memory_recall_handler(_recall_ctx(bridge), {"query": "q"}) + text = result.content[0]["text"] + + assert "MEMORY RECALL UNAVAILABLE" in text + assert "Recalled 0" not in text + assert "No relevant memories found" not in text + + @pytest.mark.asyncio + async def test_t8_handler_message_claims_no_false_cause(self, tmp_path): + """The message must describe the OUTCOME only. Every phrase below was + false for some case the same arm covers: "transient" (a misconfigured + endpoint is permanent), "proxy/auth error" and "BACKEND DOWN" (a + self-closed local transport leaves the remote blameless), "did not + respond" / "never reached" (429/5xx ARE responses, and a read failure + can happen mid-response).""" + bridge = _make_recall_bridge(tmp_path) + bridge._service.retrieve = AsyncMock(side_effect=_conn_error()) + + result = await memory_recall_handler(_recall_ctx(bridge), {"query": "q"}) + text = result.content[0]["text"] + + for false_claim in ( + "transient proxy/auth error", + "BACKEND DOWN", + "did not respond", + "never reached", + "memory is down", + ): + assert false_claim not in text, f"false cause claim in message: {false_claim}" + assert "MEMORY RECALL UNAVAILABLE" in text + assert "NOT an empty result" in text + + @pytest.mark.asyncio + async def test_t9_rate_limit_still_renders_unavailable(self, tmp_path): + """The PRE-EXISTING 429 arm. Pins that rewording the message did not + break the case it already had to cover, and that a 429 -- which IS a + response -- is not mis-described by the new text.""" + bridge = _make_recall_bridge(tmp_path) + bridge._service.retrieve = AsyncMock(side_effect=openai.RateLimitError( + "Error code: 429 - rate limit exceeded", + response=httpx.Response(429, request=_REQ), + body=None, + )) + + result = await memory_recall_handler(_recall_ctx(bridge), {"query": "q"}) + text = result.content[0]["text"] + + assert "MEMORY RECALL UNAVAILABLE" in text + assert "Recalled 0" not in text + + @pytest.mark.asyncio + async def test_t13_uninitialized_bridge_renders_unavailable(self, tmp_path): + """End-to-end artifact for T12: the guard's raise must reach the agent + as an unavailability notice, not as "No relevant memories found.".""" + bridge = MemUBridge(_make_config(tmp_path)) + bridge._available = False + # `available` is a read-only property; the handler gates only on the bridge + # being present (handlers/memory.py:67), so the guard is what fires here. + + result = await memory_recall_handler(_recall_ctx(bridge), {"query": "q"}) + text = result.content[0]["text"] + + assert "MEMORY RECALL UNAVAILABLE" in text + assert "No relevant memories found" not in text + + @pytest.mark.asyncio + async def test_t15_session_context_surfaces_the_error(self, tmp_path): + """The second production caller. It has its own broad `except` + (handlers/memory.py:253), so a raising recall must render the error text + rather than the pre-existing "(none -- fresh topic or empty memU)".""" + from nerve.agent.tools.handlers.memory import session_context_handler + + bridge = _make_recall_bridge(tmp_path) + bridge._service.retrieve = AsyncMock(side_effect=_conn_error()) + # session_context gates on `.available`, which is a read-only property + # backed by _available (already True via _make_recall_bridge). + ctx = _recall_ctx(bridge) + + result = await session_context_handler(ctx, {"topic": "t", "include_skills": False}) + text = result.content[0]["text"] + + assert "recall error" in text + assert "fresh topic or empty memU" not in text + + # --- the caller that must not crash ----------------------------------- + + @pytest.mark.asyncio + async def test_t5_pre_recall_caller_does_not_freeze_a_failure(self, tmp_path, db): + """Drives the REAL engine caller (`_get_or_create_client`), not a copy. + + `meta_updates` is set only on the success path, so a failed recall is NOT + frozen into session metadata -- `engine.py:1140` replays a frozen value on + every rebuild of the session. Session creation must also survive.""" + from nerve.agent.backends.base import BackendCapabilities + from nerve.agent.engine import AgentEngine + + cfg = _make_config(tmp_path) + cfg.workspace = tmp_path / "ws" + engine = AgentEngine(cfg, db) + await db.create_session("s-prerecall", source="web") + + bridge = _make_recall_bridge(tmp_path) + bridge._service.retrieve = AsyncMock(side_effect=_conn_error()) + engine._memory_bridge = bridge + + class _StubClient: + resume_dropped = False + native_session_id = None + model = "claude-opus-5" + + def is_alive(self): + return True + + async def disconnect(self): + pass + + class _StubBackend: + name = "claude" + capabilities = BackendCapabilities( + cost_is_cumulative=False, supports_idle_stream=False, + supports_cache_ttl=False, interactive_builtins=False, + reports_context_window=True, + ) + + def default_model(self, source): + return "claude-opus-5" + + def excluded_tools(self): + return set() + + def validate_resume_target(self, native_id, cwd): + return True + + async def create_client(self, spec): + return _StubClient() + + engine._backends["claude"] = _StubBackend() + + client = await engine._get_or_create_client("s-prerecall", "web", None) + assert client is not None, "session creation must not break" + + assert bridge._service.retrieve.await_count == 1, ( + "pre-recall never ran: the test would pass even if the branch were skipped" + ) + session = await db.get_session("s-prerecall") + meta = json.loads(session.get("metadata") or "{}") + assert "recalled_memories" not in meta, f"failed recall was FROZEN: {meta}"