[Bartok9]

Salvage of #5178 by @thalesfsp — rebased onto current main with a regression guardrail test added.

Summary

Re-raise provider errors during fact extraction instead of swallowing them and returning [].

Root Cause

Symptom — When the LLM call in _add_to_vector_store fails (HTTP 429, 5xx, connection error, timeout), add() silently returns an empty result. Callers cannot tell "LLM was unavailable" from "LLM ran fine and extracted no facts" — both look like [].

Root cause — Both the sync Memory._add_to_vector_store and async AsyncMemory._add_to_vector_store wrap self.llm.generate_response(...) in a bare except Exception that logs and return []. The exception is destroyed at the point it occurs, so no signal reaches the caller.

Evidence — mem0/memory/main.py: the sync path (logger.error("LLM extraction failed: {e}"); return []) and the async path (... (async): {e}"); return []). Confirmed present on current main (0fbbb2f). The new regression test below fails on unmodified main (the call returns [], no exception) and passes with the fix.

Fix + why this level — Replace return [] with raise in both extraction error handlers, keeping the existing logger.error. This is the correct layer: extraction is where provider transport errors first surface, and re-raising lets any wrapping retry/fallback layer (e.g. a multi-provider failover) distinguish transport failure from an empty extraction. Patching a downstream caller can't recover information already discarded here. Parse errors further down still return [] (that genuinely means "no usable facts"), so only true provider exceptions propagate.

Scope / risk — Touches only the two extraction except blocks. Behavior change: an add() that previously returned [] on a provider outage now raises. This is the intended, documented contract change; callers relying on the silent-swallow behavior should wrap in try/except. Parse-error and empty-response handling is unchanged.

Why it needed salvage

The original #5178 was conflicting against current main after pipeline refactors. Re-applied cleanly onto current main and added a guardrail test the original lacked.

Changes from original

• Rebased onto current main (single clean commit, authorship credited).
• Added test_llm_extraction_exception_is_reraised — asserts a provider error propagates; fails without the fix (returns []), passes with it.

Verification / Real behavior proof

$ python -m pytest tests/memory/test_main.py::TestAddToVectorStoreErrors -q
...                                                                      [100%]
3 passed in 0.49s

# Without the fix (main.py reverted), the new test fails as expected:
ERROR    mem0.memory.main:main.py:915 LLM extraction failed: 429 rate limit
FAILED tests/memory/test_main.py::...::test_llm_extraction_exception_is_reraised
1 failed

ruff check clean. Full tests/memory/test_main.py suite (46 tests) passes.

Credit: salvage of #5178 by @thalesfsp.

[blazingretinol]

Changes Requested

Really solid salvage — the root diagnosis is exactly right and the PR description is the clearest explanation of this bug I've seen. Two things need to land before this merges.

Strengths

• main.py:914-916 and 2538-2540: The asymmetry between the two except blocks is exactly correct. First block (LLM transport failure) now re-raises — right. Second block (parse failure at main.py:929-931) still returns [] — also right, because an LLM that responded with empty JSON did succeed; the distinction is real and the code honours it.
• The regression test uses a sentinel _ProviderError to avoid accidentally catching unrelated exceptions in pytest.raises — good technique.
• Both sync and async paths addressed in one commit; zero hot-path overhead (raise only executes on exception).
• ruff check clean; conventional commit title.

Issues

Important (should fix before merge)

1. tests/memory/test_main.py — Async path is changed but not tested

AsyncMemory._add_to_vector_store at main.py:2538-2540 gets the same return [] → raise change as the sync path, but TestAsyncAddToVectorStoreErrors has no counterpart for it. A future revert of the async raise back to return [] would pass the test suite silently. The fix is a near-copy of the existing sync test:

@pytest.mark.asyncio
async def test_async_llm_extraction_exception_is_reraised(self, mocker, mock_async_memory):
    class _ProviderError(Exception):
        pass
    mock_async_memory.llm.generate_response.side_effect = _ProviderError("429 rate limit")
    mocker.patch("mem0.memory.main.capture_event")
    with pytest.raises(_ProviderError):
        await mock_async_memory._add_to_vector_store(
            messages=[{"role": "user", "content": "test"}], metadata={}, filters={}, infer=True
        )

2. main.py:765 — LLMError contract mismatch

Memory.add() docstring declares Raises: LLMError: If LLM operations fail. but LLMError is not imported in main.py (only ValidationError is, via line 25). The bare raise propagates raw provider exceptions (openai.RateLimitError, anthropic.APIConnectionError, etc.). Any caller following the documented contract with except LLMError will not catch these. Two valid resolutions:

• Option A (preferred): Wrap at both callsites: raise LLMError(f"LLM extraction failed: {e}") from e — LLMError exists at mem0/exceptions.py:346 and supports from e for causal chaining.
• Option B: Update the docstring to Raises: Exception: and note that the raw provider exception propagates.

Note: this docstring was a dead letter pre-PR too (return [] never raised it), so this PR makes the discrepancy newly observable rather than introducing it. Either option resolves it.

Minor (optional)

• TS SDK parity: mem0-ts/src/oss/src/memory/index.ts:855-858 has the identical catch (e) { console.error(...); return []; } pattern. Not a gate for this PR (Python-only scope), but worth a follow-up.
• main.py:935 / 2558: save_messages is skipped on LLM transport failure (it only runs after successful extraction). Now that callers can see the failure, this gap is more visible — a comment noting the skip is intentional would help future readers.

Evidence

• CI: Python SDK 3.10 / 3.11 / 3.12 → PASS; CI Gate → PASS. Vercel = advisory deploy-perms fail, ignored.
• Targeted tests (with fix): pytest tests/memory/test_main.py::TestAddToVectorStoreErrors -v → 3 passed in 15.84s
• Red-green proof: impl reverted to main (return []), new test fails: FAILED ... Failed: DID NOT RAISE _ProviderError (log shows ERROR LLM extraction failed: 429 rate limit); fix restored, test passes. ✓
• Ruff: All checks passed!
• Could NOT verify: async exception propagation path through asyncio.to_thread — code tracing only (no async test yet, hence item Add simple app & query functionality #1 above).
• Duplicate cluster: PR fix(memory): re-raise LLM extraction failures instead of returning [] #5178 (original by @thalesfsp) is still open but has merge conflicts and no regression test. Once this PR merges, fix(memory): re-raise LLM extraction failures instead of returning [] #5178 can be closed.

Assessment

Ready to merge: With fixes — please add the async test and resolve the LLMError docstring. Both are quick. Happy to re-review once you push.

For tracking on our side, mind opening a short issue describing this bug and linking it here? Makes it easier to reference and prioritize follow-ups like the TS side.

[blazingretinol]

LGTM — re-review complete, ready for squash-merge

Both items from the prior review are fully addressed.

What changed in the follow-up commit

• LLMError wrapping (main.py:916, 2547): raise LLMError(f"LLM extraction failed: {e}") from e — Option A, as requested. __cause__ is preserved so callers can inspect the original provider exception.
• Async regression test (tests/memory/test_main.py:270–295): test_async_llm_extraction_exception_is_reraised added. Correctly uses effective_filters={} (matching the async signature at main.py:2449), asserts isinstance(exc_info.value.__cause__, _ProviderError) — pins the chain, not just the type.

Strengths

• main.py:914–916 and 2541–2547: The except asymmetry is exactly right — transport failure re-raises, parse failure (main.py:929–931 / 2553–2559) still returns []. The distinction is real and the code honours it.
• Memory.add() at main.py:765 already declared Raises: LLMError. This PR makes the implementation match its own docstring for the first time.
• Both tests use a sentinel _ProviderError subclass and assert __cause__ — tight regression guards that can't be gamed by coincidental raises elsewhere.
• Zero hot-path overhead — raise only executes on exception.

Issues

Important (for tracking, not a merge blocker here): The TS SDK has the identical silent-swallow at mem0-ts/src/oss/src/memory/index.ts:855-858 — catch (e) { console.error(...); return []; }. Issue #5903 (filed by the same contributor) already documents this as a follow-up. I flagged it Minor in the prior review and it's tracked, but it's worth making sure the TS fix doesn't get lost.

Minor: The async test patches EmbedderFactory.create again even though mock_async_memory's fixture already does it via _setup_mocks. The extra patch is dead code — no harm, but remove it for clarity on the next pass.

Evidence

• CI: Python SDK 3.10 / 3.11 / 3.12 → PASS; CI Gate → PASS. Vercel = advisory deploy-perms fail, ignored.
• Targeted tests (with fix): pytest tests/memory/test_main.py::TestAddToVectorStoreErrors tests/memory/test_main.py::TestAsyncAddToVectorStoreErrors -v → 6 passed in 0.67s
• Red-green proof: both new tests fail (Failed: DID NOT RAISE LLMError) with return [] restored; pass with fix ✓
• Ruff: All checks passed!
• Linked issue: LLM extraction transport failures silently swallowed (returned [] instead of raising) #5903 — PR addresses the exact reported root cause; author also documents TS follow-up there
• Cross-package: TS counterpart at index.ts:855-858 still swallows — tracked in LLM extraction transport failures silently swallowed (returned [] instead of raising) #5903, not a blocker for this Python-scoped fix
• Could NOT verify: live provider transport failures (429 / 5xx / timeout) — code tracing only; the unit test covers the mechanism completely

Assessment

Ready to merge: Yes (squash) · Est. human time: ~3 min

The fix is at the right layer, fulfils the pre-existing documented contract, both sync and async paths are tested with a tight red-green proof, and the exception chain is preserved for callers who need to inspect the original error. Please make sure #5903 gets a TS-side fix PR queued as a follow-up.

Also: once this merges, PR #5178 (the original by the credited author) has merge conflicts and no regression test — it can be closed.