Cache embedder instances to fix slow embedder loading on query endpoints

[JSv4]

Problem

Loading the configured embedder was "super duper slow" on query-time endpoints. Every embed call instantiated the embedder class fresh via embedder_class(). That constructor (PipelineComponentBase.__init__) reads the PipelineSettings DB singleton and, whenever any secret is configured, decrypts it with PipelineSettings._derive_key — a PBKDF2 KDF running 480,000 HMAC iterations (intentionally expensive). Query-time endpoints paid this DB-read + KDF cost on every request:

• Corpus.embed_text → public search API (opencontractserver/discovery/views.py)
• the MCP search_corpus tool (opencontractserver/mcp/tools.py)
• global vector search (opencontractserver/llms/vector_stores/core_vector_stores.py)

The codebase already solved this exact problem for rerankers with a process-local instance cache. This PR mirrors that established, tested pattern for embedders.

Changes

• pipeline/utils.py — new get_embedder_instance() / invalidate_embedder_cache(), caching the constructed embedder keyed by (class_path, PipelineSettings.modified). The modified (auto_now) key element means any settings write through the GraphQL mutations — which all call save() — busts the cache process-wide on the next lookup, so config changes still take effect without a worker restart.
• utils/embeddings.py + core_vector_stores.py — hot paths now use the cached instance. Per-call kwarg overrides still work (only DB-resolved defaults are pinned).
• conftest.py — extended the existing autouse cache-isolation fixture to also clear the embedder/reranker instance caches before each test, preventing cross-test collisions (the migration-seeded PipelineSettings.modified is stable across TestCase tests, so two tests mocking different classes under the same path would otherwise share a cached instance).
• test_pipeline_utils.py — new TestEmbedderInstanceCache covering per-class-path caching, distinct-path isolation, default class-path derivation, settings-write invalidation, and explicit cache invalidation.
• CHANGELOG.md — documented under Fixed.

Notes

• The MCP search tool and discovery search API benefit automatically since they route through Corpus.embed_text → generate_embeddings_from_text.
• Celery batch-ingest tasks already instantiate the embedder once per task and reuse it across a batch, so the per-call cost was already amortized there; this PR targets the per-request query endpoints.

https://claude.ai/code/session_01SE9E5SDNBRrLVc57cX7B4i

---

Generated by Claude Code

[claude]

Code Review

Overview: This PR adds a process-local instance cache for embedders, mirroring the existing reranker cache pattern in pipeline/utils.py. The motivation is clear and well-documented: PipelineComponentBase.__init__ runs a PBKDF2 KDF (480k HMAC iterations) on every instantiation, and query-time endpoints were paying this cost on every request.

---

What works well

• The double-checked locking pattern is correctly implemented — cheap dict read on the hot path, lock only on a miss.
• Cache key (class_path, PipelineSettings.modified) is a clean invalidation strategy: settings writes bust the cache automatically via auto_now, and invalidate_embedder_cache() covers the update()/bulk_update() edge cases.
• The conftest autouse fixture extension (invalidate_embedder_cache + invalidate_reranker_cache before each test) is the right place to put this — tests that mock get_component_by_name would otherwise collide on the stable migration-seeded modified timestamp.
• Test coverage is solid across the key scenarios.
• PR scope is deliberately narrow: Celery batch tasks already amortise instantiation per-task, so leaving embeddings_task.py alone is correct.

---

Issues

1. get_embedder_instance diverges from the reranker error-handling contract

The reranker's get_default_reranker_instance wraps reranker_class() in a try/except, logs a warning, and returns None on transient failures. get_embedder_instance propagates instantiation exceptions directly to callers:

# get_embedder_instance (this PR):
instance = embedder_class()   # ← propagates on failure
_EMBEDDER_INSTANCE_CACHE[cache_key] = instance
return instance

# get_default_reranker_instance (existing):
try:
    instance = reranker_class()
except Exception as e:
    logger.warning(f"Failed to instantiate reranker '{class_path}': {e}. Skipping …")
    return None   # ← degrades gracefully

In _generate_global_query_vector (core_vector_stores.py), the call is not inside a try block, so an instantiation failure propagates up. The pre-existing default_embedder_class() call would have done the same, but the reranker analogue would not — so this is a divergence rather than a regression.

Recommended fix: wrap embedder_class() in a try/except matching the reranker pattern, and return None (with a warning) on failure. Callers already handle None returns.

2. Missing get_default_embedder_instance() convenience function

The reranker has get_default_reranker_instance() — a one-call entry point that resolves the configured default class path and returns a cached instance. The embedder cache requires callers to chain get_default_embedder() → get_embedder_instance() themselves. The call in core_vector_stores.py already does this:

default_embedder_class = get_default_embedder()
query_vector = get_embedder_instance(default_embedder_class).embed_text(query_text)

A get_default_embedder_instance() would:

• complete the symmetry with the reranker API
• let callers in core_vector_stores.py collapse to a single import
• be the natural home for the error handling from issue (1) above

3. Stale entries are never evicted from the cache

When PipelineSettings.save() is called, modified bumps and the next lookup gets a new key — but the old (class_path, old_modified) entry is never removed. Each save permanently leaks one entry per cached class. The reranker cache has the same issue (#1410). For production (rare saves, typically 2-3 embedder classes), this is benign, but it's worth a brief comment alongside the module-level note (or a max-size eviction on construction if the codebase ever revisits the reranker version too).

4. Thread-safety of concrete implementations is assumed but undocumented

The base embed_text method is read-only after construction (merges kwargs into a local dict, no instance mutation). However, concrete implementations loading ML model clients or connection pools could introduce mutable state. The module-level comment says "Per-call overrides still work" but doesn't state that embedder implementations must be thread-safe to be eligible for caching. This should be documented as a requirement — possibly in BaseEmbedder's docstring — to protect future embedder authors.

---

Minor

• test_settings_write_busts_cache calls self.addCleanup(PipelineSettings._invalidate_cache) manually, but the conftest autouse fixture already calls it before every test. Not a bug, just redundant noise.
• The removed logger.debug(f"Initializing embedder instance of …") log line was useful for tracing cold-start construction; the replacement "Resolving embedder instance of …" fires on both cache hits and misses. Consider only logging on a cache miss (inside the lock block) to preserve the "construction happened" signal without spamming logs on hot paths.

---

Summary

The core implementation is correct and well-tested. The main ask is addressing the error-handling divergence from the reranker pattern (issue 1), which is worth fixing before merge to keep the two caches consistent. Adding get_default_embedder_instance() (issue 2) would also be a clean completion of the pattern but could be a follow-up. Issues 3 and 4 are low priority.

[claude]

Code Review — PR #1835: Cache embedder instances on query endpoints

Overview

This PR fixes a real performance issue: every query-time embed call was paying the PipelineComponentBase.__init__ cost (DB read + PBKDF2 KDF, ~480 k HMAC iterations) on every request. The fix mirrors the existing process-local reranker instance cache, caching instances keyed by (class_path, PipelineSettings.modified). The approach is sound and the implementation is clean.

---

What's Good

• Faithful mirror of the reranker pattern. The double-checked locking, cache-key design, invalidate_*_cache() API, and conftest.py fixture extension all match the established reranker cache exactly. Easy to audit.
• Cache invalidation is correct. Using auto_now PipelineSettings.modified as the key component means any save() through the GraphQL mutations busts the cache automatically — no manual cache-busting required for the normal settings-edit flow.
• Test isolation is handled properly. Extending _invalidate_pipeline_settings_singleton_cache to also clear the embedder/reranker instance caches prevents cross-test instance collisions where get_component_by_name is mocked to return different classes for the same path.
• Failure semantics are deliberate and documented. The embedder cache propagates construction exceptions (rather than returning None like the reranker cache does) because a None embedder would either crash downstream or silently skip the query vector. The rationale is clearly explained in the module-level block comment.
• Good test coverage. Five happy-path tests cover: per-path caching, distinct-path isolation, default path derivation, settings-write invalidation, and explicit invalidation.

---

Issues

Medium

1. No test for construction-failure propagation.
The reranker cache catches embedder_class() exceptions and returns None. The embedder cache deliberately propagates them instead — but that invariant is never tested. A test using a probe class whose __init__ raises should confirm (a) the exception propagates to the caller, and (b) the cache is NOT populated (so a subsequent call with a working class path still works).

class _FailingEmbedder(BaseEmbedder):
    def __init__(self):
        raise RuntimeError("simulated init failure")
    ...

def test_construction_failure_propagates_and_is_not_cached(self):
    from opencontractserver.pipeline.utils import get_embedder_instance
    with self.assertRaises(RuntimeError):
        get_embedder_instance(_FailingEmbedder, "tests.Failing")
    # Cache must be empty — no stale broken entry
    self.assertEqual(len(_EMBEDDER_INSTANCE_CACHE), 0)

Without this test, a future refactor that accidentally silences the exception (e.g. by adding a try/except to match the reranker pattern) would go undetected.

2. Thread-safety requirement is only in a module comment.
The module-level block comment correctly documents that "embedder implementations eligible for this cache MUST be thread-safe for read." However, get_embedder_instance() itself has no docstring warning, and BaseEmbedder has no thread-safety contract. A developer adding a new embedder that holds mutable state (e.g. a connection pool or model that isn't fork-safe) could violate this silently and only discover it under load. Consider adding a one-line note to the get_embedder_instance() docstring:

    .. warning::
        The returned instance is shared across threads. Embedder implementations
        must not mutate shared state in ``embed_text`` / ``embed_texts_batch``.

---

Minor

3. Stale cache entries accumulate on settings writes.
When PipelineSettings is saved, the old (class_path, old_modified) entry in _EMBEDDER_INSTANCE_CACHE is never evicted — only the key changes, so new lookups miss and a fresh entry is added. The old entry remains until invalidate_embedder_cache() is called explicitly. In practice, settings changes are rare and the stale value is small, so this is the same accepted limitation as the reranker cache (#1410). No action required, but worth acknowledging.

4. embedder_path explicit vs. derived key divergence.
get_embedder_instance(_CacheProbeEmbedderA, "some.path") and get_embedder_instance(_CacheProbeEmbedderA) (which derives tests.test_pipeline_utils._CacheProbeEmbedderA) are keyed differently, so the same class can have two cached instances. Not a correctness bug — both are valid instances — but a minor inefficiency if callers mix explicit and derived paths for the same class. The test suite exercises both code paths in isolation, so this is known behaviour.

5. Hot-path import inside if embedder_class: block.

# opencontractserver/utils/embeddings.py:244
from opencontractserver.pipeline.utils import get_embedder_instance

Python module imports are cached after the first call, so there's no runtime cost after warm-up. Still, it's inconsistent with the rest of the file (most imports are at module level) and slightly obscures the dependency. Consider hoisting it to the top of generate_embeddings_from_text or to module level.

---

Summary

The core change is correct and well-tested for the happy path. The two medium items are worth addressing before merge: (1) a short test for construction-failure propagation to lock in the intentional divergence from the reranker cache, and (2) a docstring note on thread-safety in get_embedder_instance(). Everything else is cosmetic or an accepted carry-over from the reranker pattern.

[claude]

Code Review

The approach is sound — mirroring the established reranker cache pattern is the right call, and the motivation is clear. A few observations below.

---

DRY violation: duplicate cache-key function

_get_embedder_cache_key and _get_reranker_cache_key are byte-for-byte identical:

def _get_pipeline_cache_key(class_path: str) -> tuple[str, Any]:
    from opencontractserver.documents.models import PipelineSettings
    modified = PipelineSettings.get_instance().modified
    return (class_path, modified)

CLAUDE.md calls this out explicitly. A single shared helper would serve both caches. Not a blocker, but worth fixing in a follow-up (or now, since the two functions were added in the same PR).

---

Subtle contract hazard: embedder_path / embedder_class mismatch

get_embedder_instance uses embedder_path as the cache key but stores an instance of embedder_class. If a caller passes a mismatched pair (e.g., get_embedder_instance(ClassA, "path.to.ClassB")), subsequent callers looking up "path.to.ClassB" will get a ClassA instance back. This is unlikely in current call sites (both places that call the function derive embedder_path from the same resolution that produced embedder_class), but a short assertion would make the contract explicit:

# Guard against caller supplying a path that doesn't match the class
assert embedder_path is None or embedder_path == (
    f"{embedder_class.__module__}.{embedder_class.__name__}"
), f"embedder_path {embedder_path!r} does not match {embedder_class}"

Or at minimum a note in the docstring warning callers not to supply a synthetic/alias path.

---

conftest.py: invalidate_reranker_cache is also new

The diff shows invalidate_reranker_cache() being added to the autouse fixture alongside the new invalidate_embedder_cache(). That means the reranker cache was also not being cleared between tests before this PR. Worth calling out as a pre-existing isolation gap that this PR quietly fixes — good catch, just not called out in the PR description.

---

Missing concurrent-access test

The double-checked locking pattern is correctly implemented (lock → recheck → construct → store), but there's no test that exercises two threads racing for the same uncached key. In practice the pattern is well-understood and the existing reranker cache has the same gap, so this is low-priority. Still, a simple ThreadPoolExecutor-based test would close the last untested branch.

---

Minor: import inside hot path

generate_embeddings_from_text now does:

from opencontractserver.pipeline.utils import get_embedder_instance

inside the if embedder_class: branch. Python caches module imports in sys.modules so the cost is a dict lookup, not a real import — no performance concern. But moving it to the top of the file (or top of the function, consistent with the rest of the module) would be cleaner and avoids any confusion about whether this is intentional deferred-loading.

---

Positive callouts

• The double-checked locking is correct — key computed once before the lock, reused for the inner check, so no TOCTOU on the key itself.
• Failure semantics (propagate vs. degrade-to-None) are correctly differentiated from the reranker cache and clearly documented.
• Test coverage is solid: caching, isolation by path, default-path derivation, settings-write invalidation, explicit invalidation, and failure-propagation-without-cache-poisoning.
• _FailingProbeEmbedder directly verifying _EMBEDDER_INSTANCE_CACHE post-failure is the right way to lock in the no-caching-on-failure invariant.

Overall this is a well-reasoned performance fix. The DRY violation and the path/class mismatch hazard are the two items most worth addressing.

[claude]

Code Review

Overview

This PR fixes a real performance problem: every query-time embed call was paying a full DB read + PBKDF2 KDF (480k iterations) to construct a fresh embedder instance. The fix mirrors the existing reranker instance cache with appropriate adaptations for embedding semantics. The implementation is sound and the test coverage is thorough.

---

Strengths

• Follows the established pattern. get_embedder_instance mirrors get_default_reranker_instance in structure (double-check locking, (class_path, PipelineSettings.modified) key, invalidate_* companion). Reviewers familiar with the reranker cache will have no surprises.
• Intentional divergence is well-justified and documented. The decision to propagate construction exceptions (vs. the reranker's None degradation) is explained in both the module-level comment and the docstring: a None embedder would move the failure downstream to a crash or silently skip the query vector. The test test_construction_failure_propagates_and_is_not_cached locks that contract in.
• Double-check locking is implemented correctly. Both the pre-lock read and the post-lock re-check are present; the instance is only stored after successful construction.
• Test isolation is properly handled. Extending the autouse _invalidate_pipeline_settings_singleton_cache fixture in conftest.py is the right approach — tests that mock get_component_by_name to return different classes under the same path won't collide on a stale cached instance.
• _get_reranker_cache_key → _get_pipeline_cache_key rename makes the shared intent clear and eliminates duplication.

---

Issues / Suggestions

1. Potential cache-key divergence between the two call sites (minor efficiency concern)

In core_vector_stores.py, get_embedder_instance is called without an explicit path, so the key is derived as f"{embedder_class.__module__}.{embedder_class.__name__}". In embeddings.py, the call passes the embedder_path string that was used to look up the class:

# core_vector_stores.py — key derived from class metadata
get_embedder_instance(default_embedder_class)

# embeddings.py — key is the lookup path, which may differ
get_embedder_instance(embedder_class, embedder_path)

If embedder_path is not identical to f"{module}.{name}" (e.g., an alias or a legacy dotted path from PipelineSettings that differs in casing or intermediate packages), the two call sites will independently cache separate instances of the same class — wasting one construction but not otherwise incorrect. The docstring warns about the mismatched-pair footgun, but this same-class, different-path edge case is distinct and worth a sentence. Consider adding a logger.debug or assert in get_embedder_instance to surface mismatches during development.

2. Pre-lock hot-path read relies on CPython GIL atomicity

# No lock held here
cached = _EMBEDDER_INSTANCE_CACHE.get(cache_key)
if cached is not None:
    return cached

dict.get() is atomic under CPython's GIL, so this is safe today. Under free-threaded CPython (PEP 703, experimental in 3.13+), this is a data race. The reranker cache has the same pattern, so this isn't a regression. Worth a brief comment (matching the reranker's phrasing) so future readers know the assumption is deliberate, not an oversight.

3. Stale-timestamp cache entries accumulate indefinitely

When PipelineSettings.modified is bumped, the old (class_path, old_modified) entries are orphaned in _EMBEDDER_INSTANCE_CACHE until invalidate_embedder_cache() is called. The reranker cache has the same behaviour. In practice modified changes rarely (only on explicit settings saves), so the accumulation is tiny. Still worth noting — a follow-up that evicts entries with stale timestamps on cache miss would be a clean improvement, though out of scope for this PR.

4. _EMBEDDER_INSTANCE_CACHE accessed directly in test (minor)

test_construction_failure_propagates_and_is_not_cached imports the private _EMBEDDER_INSTANCE_CACHE dict to assert the failed construction didn't populate it:

from opencontractserver.pipeline.utils import (
    _EMBEDDER_INSTANCE_CACHE,
    get_embedder_instance,
)
self.assertNotIn(("tests.Failing", ...), _EMBEDDER_INSTANCE_CACHE)

This is the most direct way to test the contract, and the reranker tests use similar internal-state assertions. Acceptable — just note that if the cache is ever refactored to a different data structure (e.g., a weakref or LRU dict), this test will need updating.

5. _FailingProbeEmbedder._embed_text_impl is unreachable (nit)

class _FailingProbeEmbedder(BaseEmbedder):
    def __init__(self, *args, **kwargs):
        raise RuntimeError("simulated embedder construction failure")

    def _embed_text_impl(self, text: str) -> list[float] | None:
        return [0.0] * self.vector_size  # never reached

The _embed_text_impl body can never execute because __init__ always raises. The method is required by the ABC contract, so it must be present, but a brief comment (# required by ABC; unreachable because __init__ always raises) would help readers not wonder if it's missing a raise.

---

Summary

The implementation is correct, well-tested, and consistent with the existing caching patterns in the codebase. The issues above are all minor — none of them are blockers. The cache-key divergence between call sites (#1) is the most worth a quick second look to confirm embedder_path in generate_embeddings_from_text is always f"{module}.{name}" in practice, but even if it isn't the behaviour is safe. Good to merge after a quick check on that point.