memory: bound memU embedding calls so a hung provider cannot stall it

[oranjeai]

Description

Every memU call producing an embedding runs on the "embedding" LLM profile, and that
profile was bounded by nothing: not by the bridge's asyncio.wait_for layer, not by an
httpx transport timeout, not by any caller. _instrument_llm_timeouts iterates
("memorize", "fast", "default"), so embeddings fell through to the bare OpenAI SDK
default (600s read, 2 retries) - up to ~30 minutes per call. Every other network call in
the file is bounded explicitly, and _LLM_CALL_TIMEOUT's comment already claims to cover
embed.

Affected: category seeding during agent init (inside initialize(), whose return value
engine.py discards), recall() (the query embedding is the RAG pipeline's first step),
and the runtime category create/update paths.

The bound goes on the client, where this file already puts per-call bounds, so it also
covers the memU-internal embed() sites nerve does not own. Because the seed site runs
long before _instrument_llm_timeouts(), the client is also instrumented right after the
service is constructed, before availability is published; that call is deliberately
uncaught, so a failure leaves the bridge unavailable rather than usable while unbounded.
SDK max_retries stays at its default, unlike chat, which can disable retries only
because it has its own ladder.

Two pre-existing gaps the bound would otherwise have made worse:

• recall() returned [] for a timeout, rendering as "Recalled 0 memories", because
  _is_transient_llm_error matches no timeout type - so the bound would have turned a
  stall into a wrong answer. Timeouts now report backend-down.
• _reset_llm_clients_impl evicted memorize/fast/default only, recycling three
  unrelated chat clients while keeping the transport that timed out, so "embedding" joins
  that tuple. Its warmup neighbour stays unchanged - warming an embedding client means a
  real billed request per reset.

A bounded seed is still not a correct seed: a seed-site timeout is caught by
_create_category_impl, which persists embedding = None (PR #70's territory).

Validation

15 new arms in TestEmbeddingCallTimeout; none touches a real endpoint (hangs await an
asyncio.Event never set).

embed_batch_size defaults to 1, so the 120s bound covers a whole batch, not one request -
that is why the transport timeout alone is not enough. Re-measured: 54,846 embed calls,
p99 10.0s, 21 over 30s, one at 129.7s that this bound cuts off (memorize retries).

Both directions: 13 failed / 2 passed against the unfixed source with the new tests
present, 15 passed with the fix. Most base-side failures are AttributeError, so the
real discriminator is a 19-mutant matrix - all 19 killed (13 exactly as predicted, the
rest by a superset), control green at both ends.

Full suite 2933 -> 2948 passed, 7-name pre-existing failure set byte-identical.
ruff unchanged (79 repo-wide, 0 added).

Merge coordination: #251 interacts with one assertion

Nine open PRs touch memu_bridge.py. Graded by hunk range and identifier,
#255/#254/#252/#248/#247 have no overlap, #249 inserts just above this change's anchor
(trivial), and #70 is complementary (chat-only Layer 1/2 rework, adds no embed bound).

#251 relocates self._available = True to the end of _initialize_impl - the same
goal as this change's startup ordering, reached differently. The two compose on the merits
(the bound still precedes the seed), but the ordering assertion in
test_t8_startup_call_precedes_availability_and_seeding must be re-pinned by whichever of
the two lands second. It fails loudly rather than silently.

Six of those PRs also append a new test class at the same end-of-file slot; no shared
identifiers.

[oranjeai]

Internal second-model review - 3 rounds, 21 findings adjudicated (click to expand)

Before opening this PR I ran it through an independent review loop: a cold code review plus a
second model reviewing the diff against the stated contract, then adjudication with evidence.
Three rounds ran; the first two bounced the change back for fixes. Every finding below is
listed with its verdict, including the ones I disagreed with and the two I got wrong myself.

Final round: 0 findings.

❌ Blockers and majors - all fixed

Finding	Verdict	Resolution
The change skipped no-embedding-provider installs, believing the "embedding" profile does not exist without an API key	AGREE	False against the pinned memU: LLMProfilesConfig.ensure_default always synthesizes the profile from "default", and a spy proved update_memory_item reaches .embed on it exactly once. Gate removed; that install shape is now bounded and pinned by two arms.
Nothing observed the fail-closed startup contract	AGREE	Wrapping the startup call in try/except left the suite fully green, so the contract that justifies an uncaught call was untested. Added a fresh-interpreter arm (control first, then the broken variant) plus a mutant.
A reset test asserted the opposite of production	AGREE	It claimed the embedding client is absent from the cache after a reset; measured against the real reset path it is present, re-created and wrapped. Rewritten to assert production truth on an ordered timeline.
Two arms were green for reasons unrelated to the property they pin	AGREE	Both were MagicMock vacuity: a mock auto-creates the sentinel attribute as a truthy child, which made production skip wrapping and made the assertion pass anyway. Proven by mutants that survived before and die after.
One arm claimed end-to-end update coverage without entering the update path	AGREE (remedy redesigned)	The prescribed remedy needed a second MemoryService, which raises on a re-declared column - one per process. Replaced with two cheaper links that pin the real behaviour: nerve still forwards the changed content, and memU's own resolver still maps that step to the bounded profile.
The PR body's production-latency figure was stale, and its load-bearing clause was false	AGREE	My finding, in the final round. The body said "34,874 embed calls, p99 10.9s, slowest 33.1s, none over 120s" - the sentence that licenses the 120s bound. Re-measured with two independent tools that agree: 54,846 calls, p99 10.0s, 21 over 30s, and one at 129.7s. The bound is still right (0.002% truncation, and a timed-out memorize already retries with a client reset), but the body now says so honestly instead of claiming truncation cannot happen.

⚠️ Disagreed, with evidence

Finding	Why I disagreed
The Layer 1 transport timeout is never asserted against a real AsyncOpenAI	The SDK stores self.timeout at construction and reads it per request, so a post-construction mutation is honoured by construction; and the pre-existing chat instrumentation is asserted no better, so requiring it here would be asymmetric.
docs/memory.md describes the per-call bound as covering .chat() only	Measured the convention: of the last 20 commits touching this file, one also touched that doc, and it changed user-visible provider behaviour. This adds no config surface. Noted for a later doc pass.

💡 Noted, not blocking

• One assertion in the reset arm (chat is not original) is inert: Python creates a fresh
  bound method per attribute access, so it is true whether or not anything was replaced. It was
  kept because the fix plan named that exact triple, and it cannot cause a false pass - the two
  neighbouring assertions are what the mutants die on. Recorded so nobody mistakes it for coverage.
• A recall whose transport was closed by a reset still answers empty rather than backend-down:
  a closed transport raises APIConnectionError, which matches none of the predicates. That gap
  pre-dates this change; widening the predicate has its own cost (a misconfigured base URL would
  then fail every recall loudly) and the window is narrow - the whole production log has 7 resets.
  Filed separately rather than folded in here.
• The helper docstring went from 19 lines to 7. The fix plan asked for 6 and its own prescribed
  text measures 7 - my arithmetic was wrong, not the implementation. Reflowing to 6 pushed lines
  past the file's width convention and silently changed a word, so the reviewed text was kept.
• Two other body figures were corrected in the final round after being flagged: the mutant matrix
  (16 -> 19 mutants, 10 -> 13 exact kills) and the open-PR census (eight -> nine).

On the figures

Every quantitative claim in the body was re-derived from source in the final round rather than
carried forward, including the ones no round had moved. That is what surfaced the latency defect
above: it had been correctly measured once, disclosed as "carried, not re-derived", and was wrong
by the time it mattered. Three of my own earlier claims were also refuted by re-measurement and
are corrected here rather than left standing.

[oranjeai]

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes. pytest tests/test_memu_bridge.py::TestEmbeddingCallTimeout - 15/15 deterministic, no endpoint, no sleep, no randomization. The hang is an asyncio.Event that is never set, so it is exact rather than probabilistic.
b	Root cause explained?	Yes, three measured links. The file has 4 profile-enumeration loops (:1707, :1971, :2159, :2195) and "embedding" appears in none, so the bare OpenAI SDK default governs (openai 2.44.0: read=600, max_retries=2, i.e. ~30 min). And the startup seed at :1582 embeds ~185 lines before _instrument_llm_timeouts() at :1767, so covering the loop alone would not reach it.
c	Fix matches root cause?	Yes. The bound goes on the client that lacked one, at the layer already owning per-call bounds. No widened tolerance, no disabled check, no reduced dataset, no defensive guard masking an upstream bug. It also does not paper over the seed site's pre-existing null-embedding fallback (see i).
d	Test intent preserved / new tests added?	No pre-existing test modified or weakened: the whole diff to tests/test_memu_bridge.py is +518/-0 and lives in one new class. 15 arms: 13 from this branch's first commit, plus T14 (the no-provider update path) and T15 (the fail-closed startup contract). Four arms - T5, T9, T11 and T14 - were rewritten in place rather than deleted, because each asserted something false about production or observed nothing; the coverage they carried is kept and is now inverted to the production truth. Full suite shows an identical pre-existing failure set by NAME and exactly +15 passes.
e	Both directions demonstrated?	Yes, in two arms, because the defects are of two generations. Against the pre-change source with the new tests kept: 13 failed / 2 passed; with the fix, 15 passed. Against the previous commit: 15 passed - this round is test-only, so its two defects (T11 and T14 observing nothing) are visible only through mutants M17, M18 and M19, which the previous commit's arms all survived. Because 9 of the 13 fail on AttributeError rather than on the defect (4 fail on it: T8, T9, T12, T15), the honest discriminator is a 19-mutant matrix: 19/19 killed (13 by exactly their named arm, 6 by a superset), unmutated control green at both ends, tree restoration verified against the recorded baseline tree.
f	Fix is general across code paths?	Yes. Bounding the client covers all 19 embed() sites on this branch (2 nerve-owned + 17 memU-internal, 14 of the latter reachable through MemoryService's MRO) rather than the 3 the report suggested. Symmetric paths audited: the .chat twin was already bounded (left byte-identical); the eviction loop was the sibling that lacked "embedding" and is fixed; the warmup tuple is a deliberate non-change with a recorded cost reason; _BedrockLLMClient.embed raises NotImplementedError and that profile is skipped by _inject_bedrock_clients, so there is no Bedrock carrier. Residual, deliberately not fixed: .embed on the default profile already has Layer 1 and is unreachable from nerve (update_memory_item resolves to crud.py:315 by MRO; PatchMixin is not in that MRO at all).
g	Fix generalizes across inputs (params/datatypes/wrappers)?	Yes. The wrapper is (inputs, *args, **kwargs) with return await asyncio.wait_for(...) verbatim, so it is shape-agnostic; T3 pins the (vectors, response) 2-tuple on the base client AND the vectors-only unpacking through the real LLMClientWrapper. Batch amplification is covered by design: embed_batch_size defaults to 1 and nerve never sets it, so one embed() can be N sequential requests - which is why Layer 2 bounds the call, not each request. Both provider shapes covered (T5 and T14 pin that a no-provider install is bounded too, since memU synthesizes the profile from "default") and both timeout layers (T12 runs asyncio.TimeoutError and openai.APITimeoutError); T13 pins that a ValueError still returns [].
h	Backward compatible? (maintainer-approved exception only)	Yes. No config schema change, no new user-visible option, no serialization change; LLMConfig untouched. The one intended behaviour change is narrow: a recall that times out now renders MEMORY BACKEND DOWN instead of Recalled 0 memories. Two recorded costs: a reset evicts one more client (one lazy re-construction, measured 0.019s, no I/O), and the fail-closed startup call does no I/O at all: it only reads a client object and sets attributes on it.
i	Invariants and contracts preserved?	Yes. embed()'s 2-tuple contract preserved (T3). The idempotence sentinel lives on the client, not the bridge - which is what lets both call sites run and lets the new eviction re-wrap a fresh client; a bridge-level flag would silently leave post-reset embeddings unbounded (T4, mutant M2). Ordering: eviction strictly precedes re-instrumentation, asserted positively by T9's event timeline. Per-site failure policy: fail-closed at startup (T10), best-effort on the reset path (T11). Early-return paths mutate nothing. Stated plainly: a startup timeout does not reach _ensure_categories' handler - _create_category_impl's inner except catches it, persists embedding = None and returns True, and that row is never repaired on a later boot. So this converts an unbounded stall into the pre-existing degraded outcome; it does not make seeding correct. No durability obligation - all state here is RAM-only client configuration.

Session id: cron:clickhouse-impl-slot-43:20260803-211900

[CLAassistant]

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]

cc @serxa @alex-fedotyev — could you review this? The "embedding" LLM profile was the one client with no bound: _instrument_llm_timeouts iterates ("memorize", "fast", "default"), so embeds fell through to the bare OpenAI SDK default (600s read, 2 retries), and the startup category seed embeds ~185 lines before that function runs. The bound goes on the client itself, plus a second call before availability is published so the seed is covered. Two pre-existing gaps the bound would otherwise have worsened are also closed: a timed-out recall() returned [] and rendered as "Recalled 0 memories" (835d857's rule — _is_transient_llm_error matches no timeout type), and the reset loop evicted three unrelated chat clients while keeping the transport that actually timed out.

[oranjeai]

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.