test(embedding): guard the remote-service never-loads-local invariant (D4a)

[guangyu-reflexio]

What

Regression guard for a safety invariant of the embedding subsystem: a deployment configured for the remote embedding service (REFLEXIO_EMBEDDING_DAEMON_HOST set → local_service mode) must NEVER silently load a local in-process embedder as a hidden fallback. On remote failure it must raise EmbeddingUnavailableError (→ store fail-loud / query degrade-to-FTS), not quietly instantiate NomicEmbedder/LocalEmbedder.

Why

Design item D4a / test-matrix row 8 of the embedding-stability redesign. Concretely: a self-host customer running N API instances against a shared GPU embedding service must not have a service blip trigger an N-way in-process CPU model-load stampede (wrong device, ~1GB each, fleet-wide OOM). This is the "no silent fallback" lesson from the co-located races, amplified at scale.

Invariant status: HOLDS (test-only, no source change)

Traced the path with DAEMON_HOST set + a local/* model: embedding_provider_mode() returns local_service authoritatively (no /health probe → can't re-resolve to inprocess); _post_embedding_batch raises EmbeddingUnavailableError on every HTTP failure class; _embed_texts returns from the service path before the NomicEmbedder/LocalEmbedder branches are considered. The 3× mode resolution is deterministic (pure env check with DAEMON_HOST), so no TOCTOU window. The service EmbeddingUnavailableError propagates raw (not re-wrapped as LiteLLMClientError).

Test

tests/server/llm/test_embedding_service_provider.py::TestRemoteServiceNoLocalFallback — batch + single-text paths. Each asserts the resolved mode is local_service (so it can't degrade into testing nothing), patches every local-embedder entry point (NomicEmbedder/LocalEmbedder .get/._load) to raise if invoked, forces the remote HTTP call to fail (ConnectError / 503), asserts EmbeddingUnavailableError, and assert_not_called() proves no local embedder was touched. 2 new tests; 236 existing pass; ruff + pyright clean.

Summary by CodeRabbit

• Tests
  • Added coverage for embedding requests when remote services fail, including connection issues and server-side errors.
  • Verified that local fallback paths are not used for local/* models when remote resolution is enforced.
  • Strengthened mocking to ensure failures surface as EmbeddingUnavailableError without loading in-process embedders.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR adds a new test class to the embedding service provider test suite that verifies remote embedding failures raise EmbeddingUnavailableError without falling back to in-process embedders. New imports (MagicMock, LocalEmbedder, NomicEmbedder) support patching local embedder entry points to detect any unintended fallback calls.

Changes

Remote Failure No-Fallback Test Coverage

Layer / File(s)	Summary
No-local-fallback test class and imports tests/server/llm/test_embedding_service_provider.py	Adds MagicMock, LocalEmbedder, NomicEmbedder imports, and a new TestRemoteServiceNoLocalFallback class with helpers to configure authoritative remote routing, force HTTP client failures, and assert that batch/single remote failures raise EmbeddingUnavailableError while patched local embedder methods are never called.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• ReflexioAI/reflexio#157: Prior PR makes local_service routing authoritative for local/* models, which this PR's tests directly validate.
• ReflexioAI/reflexio#273: New tests patch the refactored litellm embedding entry points introduced in this earlier PR.
• ReflexioAI/reflexio#312: Adds related coverage on local/* embedding routing and fallback behavior in the same test paths.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the test-only change and captures the remote-failure no-local-fallback invariant.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch test/remote-service-no-local-fallback

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

tests/server/llm/test_embedding_service_provider.py (1)

859-891: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Add time.sleep patch to the second test for consistency with the batch test.

The first test patches esp.time.sleep to no-op with the comment "keep the retry loop fast," indicating the retry loop uses time.sleep for backoff. The second test omits this patch. If the 5xx failure also triggers retries, this test will sleep through real backoff delays unnecessarily, slowing the suite.

♻️ Proposed fix

     self._configure_remote_service(monkeypatch)
     model = "local/nomic-embed-text-v1.5"

     assert embedding_provider_mode(model) == "local_service"

     guards = self._guard_local_embedders(monkeypatch)
+    # No backoff sleep needed — keep the retry loop fast.
+    monkeypatch.setattr(esp.time, "sleep", lambda _s: None)

     class _ServerErrorResponse:

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/server/llm/test_embedding_service_provider.py` around lines 859 - 891,
The single-text 5xx failure test in
test_single_remote_5xx_raises_and_never_loads_local should also stub out the
retry backoff sleep for consistency with the batch case. Add the same time.sleep
no-op patch used in the other test before exercising
LiteLLMClient.get_embedding, so the retry loop stays fast while still verifying
EmbeddingUnavailableError and that the local embedder guards are never called.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/server/llm/test_embedding_service_provider.py`:
- Around line 859-891: The single-text 5xx failure test in
test_single_remote_5xx_raises_and_never_loads_local should also stub out the
retry backoff sleep for consistency with the batch case. Add the same time.sleep
no-op patch used in the other test before exercising
LiteLLMClient.get_embedding, so the retry loop stays fast while still verifying
EmbeddingUnavailableError and that the local embedder guards are never called.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 5234d2af-2917-45e0-8633-fbeb0ff40884

📥 Commits

Reviewing files that changed from the base of the PR and between f94e967 and 38ef14b.

📒 Files selected for processing (1)

• tests/server/llm/test_embedding_service_provider.py