memory: never report a failed recall as "no relevant memories" by oranjeai · Pull Request #260 · ClickHouse/nerve

oranjeai · 2026-08-04T00:53:07Z

Symptom

A memory_recall whose embedding call fails at the connection layer — DNS, refused,
reset, or a transport closed under it by a client reset — renders as Recalled 0 memories /
No relevant memories found. A session concludes memory is empty, redoing work: the
"confident wrong answer" MemoryBackendUnavailable exists to prevent (835d857).

Not theoretical — Connection error. appears 42 times in this instance's 1.95 GB
nerve.log (2026-08-03 21:10 UTC), all memorize/chat-side and 0 on memU recall failed:
the class occurs routinely, recall has not drawn it yet.

Root cause

recall() enumerated transient signatures instead of partitioning failure kinds. Its only
term, _is_transient_llm_error, keys on an HTTP status (429/5xx/auth), so a failure with
no HTTP response cannot match it by construction and fell through to return [] beside logic
errors. retrieve has 0 except arms, so nothing normalizes it.

The fix

[] from recall() now means "retrieval returned nothing", for every failure it can observe:

a new _is_llm_transport_failure tests each SDK's APIConnectionError base class —
openai and anthropic are disjoint hierarchies, _BedrockLLMClient raises the latter,
and each APITimeoutError subclasses its own, covering timeouts;
infrastructure failures raise MemoryBackendUnavailable; everything else propagates to
the caller's pre-existing generic arm, not swallowed;
the uninitialized-bridge guard raises instead of returning [];
the message drops its false cause ("transient proxy/auth error") for an outcome-only one —
the same arm covers 429s, which are responses.

A genuine miss still returns [], pinned by a test. All three callers already catch, and
engine.py's pre-recall stops freezing a failed recall into session metadata, replayed
on every rebuild.

Validation

14 fail / 1 pass at base (the 1 is the genuine-miss control) vs 15 pass fixed; a
17-mutant matrix kills every mutant. Whole suite 2933 -> 2948, identical failed set.

Scope, and why [] is not globally honest

The widening is recall() only. The write and category paths keep their return []/False: their tool layers already render an explicit failure (Failed to update memory <id>, NOT saved - MEMORY BACKEND UNAVAILABLE), so they produce no wrong answer — only
a read reporting emptiness lies. memorize_file already raises for 429/5xx.

[] is honest for everything nerve observes, not globally: memU absorbs a malformed LLM
ranking response below this arm (memu/app/retrieve.py:1344 logs and returns an empty
list), so on the method="llm" path a garbled ranker reply still yields a legitimate-looking
empty recall and no exception ever reaches recall(). memu-py is pinned, so closing that
is a memU-layer change - out of scope, named here so the docstring is not read as the
stronger claim.

Raw httpx errors are deliberately untested: both SDKs wrap them
(openai/_base_client.py:1683, anthropic/_base_client.py:2061), and nerve sets
client_backend="sdk" at every profile, so HTTPLLMClient is never constructed — that
family has no carrier.

Mutation matrix: 17 mutants, all killed, unmutated control green at both ends, including
M12 (residual returns empty - under-classification) and M13/M15 (over-classification).
The 7 pre-existing whole-suite failures are timezone off-by-ones in
TestResolveEventDatesSync plus one in test_telegram_sessions, unchanged by this PR;
ruff finding set identical modulo line shifts.

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.

oranjeai · 2026-08-04T00:53:14Z

Internal second-model review (3 rounds, 20 findings, all adjudicated)

Before opening this PR I ran my own cold code review plus an independent second-model
review (codex) against the resulting code, over 3 rounds. Every finding is listed with
its verdict, including the ones I disagreed with and why. 0 verdict reversals; no finding
was raised twice.

Fixed in this PR

#	finding	what was wrong	how it was proved
1	`engine-recall-test`	The pre-recall regression test rebuilt `engine.py`'s block as local variables, so the production caller and its real `update_session_metadata` write never ran; the assertion was about the test's own dict.	Rewritten to drive the real `_get_or_create_client` and read the session row back from the DB. A mutant that makes the engine's arm freeze the failed recall survives the old test tree and dies against the new one.
2	`engine-recall-attempt`	Even after that repair the test still passed if pre-recall was skipped entirely: "client created" and "metadata absent" are also the outcomes when the branch never executes.	A mutant gating the `elif` to `False` survived at 15 passed / 0 failed. Fixed by asserting the injected mock was awaited; that mutant now kills exactly that arm.
3	`genuine-miss-attempt`	The genuine-miss control, the arm that keeps the re-raise inversion honest, could not distinguish a successful empty retrieval from a skipped one: its only assertion holds for any early `[]`.	A mutant returning `[]` before retrieval killed 12 arms but left the miss control passing. Fixed with an invocation assertion; that mutant now kills it.
4	`outcome-message-down`	The message is meant to describe the outcome, not the cause, and its own test rejects "BACKEND DOWN" as a false claim, yet it still ended "alert the operator that memory is down" -- false for two cases the same arm covers (a locally self-closed transport, and a bridge that never initialized). The case-sensitive oracle could not see the lowercase form.	Reworded to an outcome statement, and the missing needle added to the false-claim oracle so it covers the class it claims to. Reverting the reword now fails that arm.
5	`recall-comment-volume`	New docstrings and comments carried PR rationale and dependency-internal line numbers into production source -- citations that rot silently on a dependency bump.	Condensed to the outcome invariant plus the dual-SDK rationale (-20/+12 lines); the scope adjudication and dependency references moved to the PR body.
6	`session-context-arm-unasserted`	The second production caller (`session_context`) had no test making its recall raise, so the claim that it surfaces the error was unverified.	An arm was added; a mutant that makes that handler render the pre-existing empty text now kills it.
7	`pr-body-figures-stale-r1`	A fix round changed five figures the PR description quoted (arm counts, suite total, matrix size), leaving the description describing an earlier tree.	Re-derived each from the round's own logs and corrected.
8	`ai-gate-figures-unreproducible`	Two of my own figures did not reproduce: the count of broad `except` arms had drifted to 52-53 across rounds, and one embedding call-site count was off.	Re-measured by AST: 50 at both base and head. Both load-bearing conclusions were independently re-verified and stand.
9	`pr-body-mutant-count-stale-r2`	The final round added three mutants, making the description's "14-mutant matrix" wrong in two places while another comment already said 17 -- the PR would have contradicted itself across two published surfaces.	Re-derived from the mutation log: 17 distinct mutants, all killed. Both stale figures understated coverage. Corrected before the final gate ran, so the gate reviewed the corrected description.
10	`embed-site-count-off-by-one-r2`	A claim placed five embedding call sites "in `retrieve`"; by AST that function contains none of them -- they live in the pipeline steps it dispatches.	Corrected to name the true locus with line numbers. Every other figure in that row reproduced exactly.

Disagreed, with evidence

finding	why I disagreed
`engine-freeze-success-side-unpinned` (mine)	Nothing pins the positive direction of the pre-recall freeze -- that a successful recall still writes the key, the deliberate prompt-cache behaviour. A real gap, but the base had zero coverage of that key either, and this change's contract is failure classification. Named rather than silently widened.
`register-handler-not-unregistered-in-t5` (mine)	The new engine test leaks one entry into the module-global interactive-handler registry. It follows the repo's own existing precedent for this shape, the key is unique to the test, and the whole suite is green with a failed-name set identical to base. Not this change's convention to alter.
`stray-magicmock-dir-untracked` (mine)	Harness artifact: 0 files / 88 empty dirs, untracked, absent from the commit, produced by running pytest -- present in unrelated checkouts and absent from one that never ran the suite.
`openai-not-a-declared-dependency` (mine)	The new test-only import arrives transitively from the hard-pinned `memu-py`, which imports it unconditionally, so any install where these tests run has it. The production predicate keeps its guarded import, so no source coupling is added.

Noted, not blocking

Test-docstring volume. The new test class carries more narrative than this repository's
convention prefers: 47 docstring lines against 245 body lines (19.2%), where the
next-busiest class in the same file is 5.9% and 12 of 17 are at 0.0%. Three spots restate
the symptom-and-fix story, embed a production occurrence count, and enumerate every
rejected wording. The equivalent finding on the production source was fixed in an
earlier round; this is the same class one file over, and it is tracked as a follow-up
rather than bounced, since no blocking finding remained. The embedded count is a lower
bound and is still true, so the defect is invited future drift, not a false statement.

Three coverage observations, all on lines this change does not touch, each demonstrated
by a surviving mutant and each recorded rather than papered over: the guard's
service-object conjunct is unpinned (that state is close to unreachable, and the guard's
changed behaviour is pinned); the exception-cause chain is unpinned (every arm matches on
a message substring the mutant still satisfies); and the success-side freeze is unpinned as
noted above. Every direction of the behaviour this change does alter is pinned: all 15
arms are killed by at least one mutant, with no arm left without a killer.

One residual is deliberately out of scope and stated in the PR description rather than
papered over: memU can absorb a malformed ranking response below this arm, so an empty
recall is honest for everything this code observes, not globally.

oranjeai · 2026-08-04T00:53:15Z

Pre-PR validation gate (a-i)

#	check	answer
a	Deterministic repro?	Yes. Classification is a pure function of the exception object -- no timing, no randomization, not a "~1%" case. Two independent reproductions: a 15-input predicate matrix (`BAD=0`), and end-to-end through the real `recall()` + the real tool handler, where the base tree renders `No relevant memories found.` for `openai.APIConnectionError`.
b	Root cause explained?	Yes. The embedding transport fails without a response -> `openai` raises `APIConnectionError` after its own retries -> memU re-raises untouched (`memu/app/retrieve.py:42 retrieve` has 0 `except` arms, verified by AST) -> `_is_transient_llm_error` keys on an HTTP status and so cannot match by construction -> `return []` -> the tool layer prints a count.
c	Fix matches the root cause?	Yes -- it repairs the partition (a base-class test across both SDK connection-error hierarchies, plus the plain-`asyncio` timeout term) rather than appending a special case. Not a band-aid: the tempting narrow alternative (key on a `__cause__` message for `client has been closed`) is mutant M11, killed by 4 arms, because it fixes the 0-occurrence sub-case and leaves the 42-occurrence one broken.
d	Test intent preserved / new tests added?	Yes. 15 new arms; nothing weakened or removed. Two drive the real production callers rather than a copy of their logic (the engine pre-recall path via `_get_or_create_client` plus the persisted session row, and `session_context`). T14 pins that a genuine miss still returns `[]` (else the fix would make every miss look like an outage); T9 pins the pre-existing 429 case so the message rewording did not break the case it already covered.
e	Demonstrated in BOTH directions?	Yes. Base source + new tests: 14 failed / 1 passed (the 1 is the genuine-miss control, which must hold on both trees). Fixed: 15 passed. Plus a 17-mutant matrix, every mutant killed, each hitting exactly its predicted arm set, unmutated control green at both ends, `TREE_RESTORED_OK`. Five mutants target the real production callers or the operator message and were measured to survive an earlier test tree while being killed by the current one, so every added assertion's coverage is demonstrated, not asserted.
f	General across CODE paths?	Yes. Enumerated all 50 broad `except` arms by AST (bare/`Exception`/`BaseException`, same count at HEAD and at base `94406ea`). After the fix exactly two read paths still `return []` -- `list_items` and `list_categories` -- and both are non-network-bearing, verified mechanically rather than assumed: within the two callee function bodies (`memu/app/crud.py:38,59`) there are 0 `embed`/`llm`/`client` references (whole-file `crud.py` has many; the claim is function-scoped), against 5 `embed_client.embed` call sites on the recall path (in the pipeline steps `retrieve` dispatches, `memu/app/retrieve.py:271,321,357,397,421`), so a pure DB read cannot fail for transport reasons. Fixed at the source (the classifier), not guarded at the render site.
g	Generalizes across INPUTS?	Yes, `BAD=0` over three axes: subclass leaves of both SDK families (so the fix cannot rot as an SDK adds leaves); 10 `__cause__` shapes (None, closed transport, refused, DNS, `RemoteProtocolError`, read/write/pool errors, `OSError`, `ValueError`) -- all classify, since the predicate reads the type, never a message; and 6 degenerate inputs (`BaseException`, `KeyboardInterrupt`, `SystemExit`, bare `Exception`, `GeneratorExit`, and an exception whose `__str__` raises) -- all `False`, none raised.
h	Backward compatible?	Yes -- no setting, no serialization format, no config key, no feature gate, no DB migration. The only behaviour change is which of two existing tool messages an agent sees. Import safety verified in 5 arms: with `anthropic` absent the `openai` arm still works; with both absent the predicate returns `False` without raising; a renamed SDK attribute is caught; and no module-level SDK import is added.
i	Invariants and contracts preserved?	Yes, and the invariant is stated scoped: no failure that reaches `MemUBridge.recall` is reported as `[]` -- deliberately not the global claim "`[]` always means a successful miss", because memU absorbs a malformed LLM ranking response below this arm (`memu/app/retrieve.py:1341-1347`, `memu-py==1.4.0` pinned), so a garbled ranker reply still yields a legitimate empty recall with no exception reaching us. Closing that is a memU-layer change, out of scope, named so nobody is misled. Re-raise safety verified at all three callers (each already catches broadly), so the inversion cannot escape unhandled. A predicate bug must surface, not degrade to `[]`: the `try` wraps only the import, never the `isinstance` -- proven with a metaclass whose `__instancecheck__` raises, which propagates.

Deliberate scope exclusions (decisions, not omissions): 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; the two category paths are also
owned by an in-flight sibling review. _is_llm_timeout stays in the file with its
memorize_file call site intact -- only recall's arm no longer calls it, since each SDK's
APITimeoutError is a subclass of its APIConnectionError (pinned by an arm, so a future
reparenting fails loudly).

Predicted mutation survival, recorded rather than papered over: reverting the
MemoryBackendUnavailable docstring leaves the suite green (15/15 since r1 added an arm).
A docstring has no runtime observer, so no test can kill it; asserting on docstring text would
be pinning prose, not behaviour.

Not applicable: no 50/50 randomized-run plan and no Build ID -- this is a Python change in
this repo, not a ClickHouse test.

CLAassistant · 2026-08-04T00:53:17Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

oranjeai · 2026-08-04T04:05:19Z

Closing per @pufit's directive on #247: memU is being rewritten and sunset, and Nerve fixes
outside "critical performance problem" or "makes my work easier" are handled by the Nerve team.
This PR is a correctness fix in neither category, so it is closed unmerged. The analysis stays in
the description and comments if it is useful during the rewrite. No further action needed from me.

oranjeai closed this Aug 4, 2026

oranjeai mentioned this pull request Aug 4, 2026

memory: make the event-date sweep's writes atomic so concurrent writers are not clobbered #247

Closed

Navigation Menu

Uh oh!

memory: never report a failed recall as "no relevant memories" - #260

memory: never report a failed recall as "no relevant memories"#260
oranjeai wants to merge 1 commit into
ClickHouse:mainfrom
oranjeai:oranjeai/recall-transport-classification

oranjeai commented Aug 4, 2026

Uh oh!

oranjeai commented Aug 4, 2026

Fixed in this PR

Disagreed, with evidence

Noted, not blocking

Uh oh!

oranjeai commented Aug 4, 2026

Uh oh!

CLAassistant commented Aug 4, 2026

Uh oh!

oranjeai commented Aug 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

oranjeai commented Aug 4, 2026

Symptom

Root cause

The fix

Validation

Uh oh!

oranjeai commented Aug 4, 2026

Fixed in this PR

Disagreed, with evidence

Noted, not blocking

Uh oh!

oranjeai commented Aug 4, 2026

Uh oh!

CLAassistant commented Aug 4, 2026

Uh oh!

oranjeai commented Aug 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants